Re: [HACKERS] logical replication - still unstable after all these months

2017-05-29 Thread Petr Jelinek
On 29/05/17 03:33, Jeff Janes wrote:
> On Sun, May 28, 2017 at 3:17 PM, Mark Kirkwood
> <mark.kirkw...@catalyst.net.nz <mailto:mark.kirkw...@catalyst.net.nz>>
> wrote:
> 
> The framework ran 600 tests last night, and I see 3 'NOK' results,
> i.e 3 failed test runs (all scale 25 and 8 pgbench clients). Given
> the way the test decides on failure (gets tired of waiting for the
> table md5's to match) - it begs the question 'What if it had waited
> a bit longer'? However from what I can see in all cases:
> 
> - the rowcounts were the same in master and replica
> - the md5 of pgbench_accounts was different
> 
> 
> All four tables should be wrong if there is still a transaction it is
> waiting for, as all the changes happen in a single transaction.  

Not necessarily, if the bug is in the sync worker or in the sync to
apply handover code (which is one of the more complicated parts of all
of the logical replication, so it's prime candidate) then it can easily
be just one table.

> I also got a failure, after 87 iterations of a similar test case.  It
> waited for hours, as mine requires manual intervention to stop waiting. 
> On the subscriber, one account still had a zero balance, while the
> history table on the subscriber agreed with both history and accounts on
> the publisher and the account should not have been zero, so definitely a
> transaction atomicity got busted.

I am glad others are able to reproduce this, my machine is still at 0
failures after 800 cycles.

> 
> What would you want to look at?  Would saving the WAL from the master be
> helpful?
> 

Useful info is, logs from provider (mainly the logical decoding logs
that mention LSNs), logs from subscriber (the lines about when sync
workers finished), contents of the pg_subscription_rel (with srrelid
casted to regclass so we know which table is which), and pg_waldump
output around the LSNs found in the logs and in the pg_subscription_rel
(+ few lines before and some after to get context). It's enough to only
care about LSNs for the table(s) that are out of sync.

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] logical replication busy-waiting on a lock

2017-05-27 Thread Petr Jelinek
On 27/05/17 15:44, Petr Jelinek wrote:
> On 27/05/17 01:25, Jeff Janes wrote:
>> When I create a subscription in the disabled state, and then later doing
>> "alter subscription sub enable;", on the master I sometimes get a tight
>> loop of the deadlock detector:
>>
>> (log_lock_waits is on, of course)
>>
>> deadlock_timeout is set to 1s, so I don't know why it seems to be
>> running several times per millisecond.
>>
>> .
>>
>> And so on out to "after 9616.814", when it finally acquires the lock.
>>
>> The other process, 47457, is doing the initial COPY of another table as
>> part of the same publisher/subscriber set.
>>
> 
> We lock wait for running transactions in snapshot builder while the
> snapshot is being built so I guess that's what you are seeing. I am not
> quite sure why the snapshot builder would hold the xid lock for
> prolonged period of time though, the XactLockTableWait releases the lock
> immediately after acquiring it. In fact AFAICS everything that acquires
> ShareLock on xid releases it immediately after acquiring as it's only
> used for waits.
> 

Actually, I guess it's the pid 47457 (COPY process) who is actually
running the xid 73322726. In that case that's the same thing Masahiko
Sawada reported [1]. Which basically is result of snapshot builder
waiting for transaction to finish, that's normal if there is a long
transaction running when the snapshot is being created (and the COPY is
a long transaction).

[1]
https://www.postgresql.org/message-id/flat/CAD21AoC2KJdavS7MFffmSsRc1dn3Vg_0xmuc%3DUpBrZ-_MUxh-Q%40mail.gmail.com

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] logical replication busy-waiting on a lock

2017-05-27 Thread Petr Jelinek
On 27/05/17 01:25, Jeff Janes wrote:
> When I create a subscription in the disabled state, and then later doing
> "alter subscription sub enable;", on the master I sometimes get a tight
> loop of the deadlock detector:
> 
> (log_lock_waits is on, of course)
> 
> deadlock_timeout is set to 1s, so I don't know why it seems to be
> running several times per millisecond.
> 
> .
> 
> And so on out to "after 9616.814", when it finally acquires the lock.
> 
> The other process, 47457, is doing the initial COPY of another table as
> part of the same publisher/subscriber set.
> 

We lock wait for running transactions in snapshot builder while the
snapshot is being built so I guess that's what you are seeing. I am not
quite sure why the snapshot builder would hold the xid lock for
prolonged period of time though, the XactLockTableWait releases the lock
immediately after acquiring it. In fact AFAICS everything that acquires
ShareLock on xid releases it immediately after acquiring as it's only
used for waits.

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] ALTER SUBSCRIPTION ..SET PUBLICATION refresh is not throwing error.

2017-05-27 Thread Petr Jelinek
On 27/05/17 04:00, Euler Taveira wrote:
> 2017-05-26 21:29 GMT-03:00 Petr Jelinek <petr.jeli...@2ndquadrant.com
> <mailto:petr.jeli...@2ndquadrant.com>>:
> 
> 
> Actually another possibility would be to remove the REFRESH keyword
> completely and just have [ WITH (...) ] and have the refresh option
> there, ie simplified version of what you have suggested (without the
> ugliness of specifying refresh twice to disable).
> 
> 
> It will cause confusion. It seems that WITH sets ALTER SUBSCRIPTION
> properties. Indeed, they are REFRESH properties. I think we shouldn't
> exclude REFRESH keyword. Syntax leaves no doubt that WITH are REFRESH
> properties.
> 

Maybe, I don't know, it might not be that confusing when SET PUBLICATION
and REFRESH PUBLICATION have same set of WITH options.

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] ALTER SUBSCRIPTION ..SET PUBLICATION refresh is not throwing error.

2017-05-26 Thread Petr Jelinek
On 27/05/17 02:13, Euler Taveira wrote:
> 2017-05-26 17:58 GMT-03:00 Peter Eisentraut
> <peter.eisentr...@2ndquadrant.com
> <mailto:peter.eisentr...@2ndquadrant.com>>:
> 
> On 5/24/17 15:38, Petr Jelinek wrote:
> >>> I wonder if we actually need the SKIP REFRESH syntax, there is the
> >>> "REFRESH [ WITH ... ]" when user wants to refresh, so if REFRESH is 
> not
> >>> specified, we can just behave as if SKIP REFRESH was used, it's not 
> like
> >>> there is 3rd possible behavior.
> >>
> >> Attached patch does exactly that.
> >
> > And of course I forgot to update docs...
> 
> Do we want not-refreshing to be the default behavior?
> 
> 
> It is a different behavior from the initial proposal. However, we
> fortunately have ALTER SUBSCRIPTION foo REFRESH PUBLICATION and can
> refresh later. Also, if "refresh" is more popular than "skip", it is
> just a small word in the command. That's the price we pay to avoid
> ambiguity that the previous syntax had.At least I think Petr's proposal
> is less confusing than mine (my proposal maintains current behavior but
> can cause some confusion).
> 

Actually another possibility would be to remove the REFRESH keyword
completely and just have [ WITH (...) ] and have the refresh option
there, ie simplified version of what you have suggested (without the
ugliness of specifying refresh twice to disable).

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] logical replication - still unstable after all these months

2017-05-26 Thread Petr Jelinek
On 26/05/17 16:51, Alvaro Herrera wrote:
> Erik Rijkers wrote:
> 
>> I wouldn't say that problems (re)appeared at a certain point; my impression
>> is rather that logical replication has become better and better.  But I kept
>> getting the odd failure, without a clear cause, but always (eventually)
>> repeatable on other machines.  I did the 1-minute pgbench-derail version
>> exactly because of the earlier problems with snapbuild: I wanted a test that
>> does a lot of starting and stopping of publication and subscription.
> 
> I think it is pretty unlikely that the logical replication plumbing is
> the buggy place.  You're just seeing it now becaues we didn't have any
> mechanism as convenient to consume logical decoding output.  In other
> words, I strongly suspect that the hyphothetical bugs are in the logical
> decoding side (and snapbuild sounds the most promising candidate) rather
> than logical replication per se.
> 

Well, that was true for the previous issues Erik found as well (mostly
snapshot builder was problematic). But that does not mean there are no
issues elsewhere. We could do with some more output from the tests (do
they log some intermediary states of those md5 checksums, maybe numbers
of rows etc?), description of the problems, errors from logs, etc. I for
example don't get any issues from similar test as the one described in
this thread so without more info it might be hard to reproduce and fix
whatever the underlaying issue is.

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] logical replication - still unstable after all these months

2017-05-26 Thread Petr Jelinek
Hi,

Hmm, I was under the impression that the changes we proposed in the
snapbuild thread fixed your issues, does this mean they didn't? Or the
modified versions of those that were eventually committed didn't? Or did
issues reappear at some point?

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Create subscription with `create_slot=false` and incorrect slot name

2017-05-25 Thread Petr Jelinek
On 25/05/17 23:26, Peter Eisentraut wrote:
> On 5/24/17 21:41, Robert Haas wrote:
>>> This came up in a previous thread.  It is up to the publishing end what
>>> slot names it accepts.  So running the validation locally is incorrect.
>>
>> That argument seems pretty tenuous; surely both ends are PostgreSQL,
>> and the rules for valid slot names aren't likely to change very often.
> 
> Remember that this could be used for upgrades and side-grades, so the
> local rules could change or be more restricted in the future or
> depending on compilation options.
> 
>> But even if we accept it as true, I still don't like the idea that a
>> DROP can just fail, especially with no real guidance as to how to fix
>> things so it doesn't fail.  Ideas:
>>
>> 1. If we didn't create the slot (and have never connected to it?),
>> don't try to drop it.
> 
> That would conceptually be nice, but it would probably create a bunch of
> problems of its own.  For one, we would need an interlock so that the
> first $anything that connects to the slot registers it in the local
> catalog as "it's mine now".
> 
>> 2. Emit some kind of a HINT telling people about ALTER SUBSCRIPTION ..
>> SET (slot_name = NONE).
> 
> The reported error is just one of many errors that can happen when DROP
> SUBSCRIPTION tries to drop the slot (doens't exist, still active, no
> permission, etc.).  We don't want to give the hint that is effectively
> "just forget about the slot then" for all of them.  So we would need
> some way to distinguish "you can't do this right now" from "this would
> never work" (400 vs 500 errors).
> 

This specific error returns ERRCODE_UNDEFINED_OBJECT error code so we
could check for it and offer hint only for this case.



-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] No parameter values checking while creating Alter subscription...Connection

2017-05-25 Thread Petr Jelinek
On 25/05/17 23:18, Andres Freund wrote:
> On 2017-05-25 17:08:57 -0400, Peter Eisentraut wrote:
>> On 5/25/17 10:18, Masahiko Sawada wrote:
>>>> postgres=# alter subscription c1 connection 'port=4000';
>>>> ALTER SUBSCRIPTION
>>>> postgres=# alter subscription c1 connection 'dbname=cc';
>>>> ALTER SUBSCRIPTION
>>>>
>>> CREATE SUBSCRIPTION tries to connect to publisher to create
>>> replication slot or to get table list for table synchronization, not
>>> to check the connection parameter value. So if you specify connect =
>>> false then CREATE SUBSCRIPTION doesn't try to connect.
>>
>> We don't make a connection attempt as part of ALTER SUBSCRIPTION.  I
>> guess we could just connect and disconnect to check that it works.
> 
> I think during reconfigurations it's quite useful to be able to do so
> even if the other hosts aren't reachable that second.
> 

Yes, it's intended behavior for this very reason, we want ability to
(re)configure downstream without any existing upstream. It's also reason
why we have the WITH (connect = false) in CREATE SUBSCRIPTION. I don't
see a nice way how to do something similar (ie make it optional) for
ALTER though.

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


[HACKERS] Walsender timeouts and large transactions

2017-05-25 Thread Petr Jelinek
Hi,

We have had issue with walsender timeout when used with logical decoding
and the transaction is taking long time to be decoded (because it
contains many changes)

I was looking today at the walsender code and realized that it's because
if the network and downstream are fast enough, we'll always take fast
path in WalSndWriteData which does not do reply or keepalive processing
and is only reached once the transaction has finished by other code. So
paradoxically we die of timeout because everything was fast enough to
never fall back to slow code path.

I propose we only use fast path if the last processed reply is not older
than half of walsender timeout, if it is then we'll force the slow code
path to process the replies again. This is similar logic that we use to
determine if to send keepalive message. I also added CHECK_INTERRUPRS
call to fast code path because otherwise walsender might ignore them for
too long on large transactions.

Thoughts?

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


0001-Fix-walsender-timeouts-when-decoding-large-transacti.patch
Description: binary/octet-stream

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] ALTER SUBSCRIPTION ..SET PUBLICATION refresh is not throwing error.

2017-05-24 Thread Petr Jelinek
On 24/05/17 21:28, Petr Jelinek wrote:
> On 24/05/17 21:21, Petr Jelinek wrote:
>> On 24/05/17 21:07, Euler Taveira wrote:
>>> 2017-05-23 6:00 GMT-03:00 tushar <tushar.ah...@enterprisedb.com
>>> <mailto:tushar.ah...@enterprisedb.com>>:
>>>
>>>
>>> s=# alter subscription s1 set publication  skip refresh ;
>>> NOTICE:  removed subscription for table public.t
>>> NOTICE:  removed subscription for table public.t1
>>> ALTER SUBSCRIPTION
>>> s=#
>>>
>>>
>>> That's a design flaw. Since SKIP is not a reserved word, parser consider
>>> it as a publication name. Instead of causing an error, it executes
>>> another command (REFRESH) that is the opposite someone expects. Also, as
>>> "skip" is not a publication name, it removes all tables in the subscription.
>>>
>>
>> Ah that explains why I originally added the ugly NOREFRESH keyword.
>>
>>> ALTER SUBSCRIPTION name SET PUBLICATION publication_name_list SKIP REFRESH
>>> ALTER SUBSCRIPTION name SET PUBLICATION publication_name_list REFRESH
>>> opt_definition
>>>
>>> I think the first command was a bad design. Why don't we transform SKIP
>>> REFRESH into a REFRESH option?
>>>
>>> ALTER SUBSCRIPTION sub1 SET PUBLICATION pub1 REFRESH WITH (skip = true);
>>>
>>> skip (boolean): specifies that the command will not try to refresh table
>>> information. The default is false.
>>
>> That's quite confusing IMHO, saying REFRESH but then adding option to
>> actually not refresh is not a good interface.
>>
>> I wonder if we actually need the SKIP REFRESH syntax, there is the
>> "REFRESH [ WITH ... ]" when user wants to refresh, so if REFRESH is not
>> specified, we can just behave as if SKIP REFRESH was used, it's not like
>> there is 3rd possible behavior.
>>
> 
> Attached patch does exactly that.
> 

And of course I forgot to update docs...

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


Remove-the-SKIP-REFRESH-syntax-suggar-in-ALTER-SUBSC-v2.patch
Description: binary/octet-stream

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] ALTER SUBSCRIPTION ..SET PUBLICATION refresh is not throwing error.

2017-05-24 Thread Petr Jelinek
On 24/05/17 21:21, Petr Jelinek wrote:
> On 24/05/17 21:07, Euler Taveira wrote:
>> 2017-05-23 6:00 GMT-03:00 tushar <tushar.ah...@enterprisedb.com
>> <mailto:tushar.ah...@enterprisedb.com>>:
>>
>>
>> s=# alter subscription s1 set publication  skip refresh ;
>> NOTICE:  removed subscription for table public.t
>> NOTICE:  removed subscription for table public.t1
>> ALTER SUBSCRIPTION
>> s=#
>>
>>
>> That's a design flaw. Since SKIP is not a reserved word, parser consider
>> it as a publication name. Instead of causing an error, it executes
>> another command (REFRESH) that is the opposite someone expects. Also, as
>> "skip" is not a publication name, it removes all tables in the subscription.
>>
> 
> Ah that explains why I originally added the ugly NOREFRESH keyword.
> 
>> ALTER SUBSCRIPTION name SET PUBLICATION publication_name_list SKIP REFRESH
>> ALTER SUBSCRIPTION name SET PUBLICATION publication_name_list REFRESH
>> opt_definition
>>
>> I think the first command was a bad design. Why don't we transform SKIP
>> REFRESH into a REFRESH option?
>>
>> ALTER SUBSCRIPTION sub1 SET PUBLICATION pub1 REFRESH WITH (skip = true);
>>
>> skip (boolean): specifies that the command will not try to refresh table
>> information. The default is false.
> 
> That's quite confusing IMHO, saying REFRESH but then adding option to
> actually not refresh is not a good interface.
> 
> I wonder if we actually need the SKIP REFRESH syntax, there is the
> "REFRESH [ WITH ... ]" when user wants to refresh, so if REFRESH is not
> specified, we can just behave as if SKIP REFRESH was used, it's not like
> there is 3rd possible behavior.
> 

Attached patch does exactly that.

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


0001-Remove-the-SKIP-REFRESH-syntax-suggar-in-ALTER-SUBSC.patch
Description: binary/octet-stream

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] ALTER SUBSCRIPTION ..SET PUBLICATION refresh is not throwing error.

2017-05-24 Thread Petr Jelinek
On 24/05/17 21:07, Euler Taveira wrote:
> 2017-05-23 6:00 GMT-03:00 tushar <tushar.ah...@enterprisedb.com
> <mailto:tushar.ah...@enterprisedb.com>>:
> 
> 
> s=# alter subscription s1 set publication  skip refresh ;
> NOTICE:  removed subscription for table public.t
> NOTICE:  removed subscription for table public.t1
> ALTER SUBSCRIPTION
> s=#
> 
> 
> That's a design flaw. Since SKIP is not a reserved word, parser consider
> it as a publication name. Instead of causing an error, it executes
> another command (REFRESH) that is the opposite someone expects. Also, as
> "skip" is not a publication name, it removes all tables in the subscription.
> 

Ah that explains why I originally added the ugly NOREFRESH keyword.

> ALTER SUBSCRIPTION name SET PUBLICATION publication_name_list SKIP REFRESH
> ALTER SUBSCRIPTION name SET PUBLICATION publication_name_list REFRESH
> opt_definition
> 
> I think the first command was a bad design. Why don't we transform SKIP
> REFRESH into a REFRESH option?
> 
> ALTER SUBSCRIPTION sub1 SET PUBLICATION pub1 REFRESH WITH (skip = true);
> 
> skip (boolean): specifies that the command will not try to refresh table
> information. The default is false.

That's quite confusing IMHO, saying REFRESH but then adding option to
actually not refresh is not a good interface.

I wonder if we actually need the SKIP REFRESH syntax, there is the
"REFRESH [ WITH ... ]" when user wants to refresh, so if REFRESH is not
specified, we can just behave as if SKIP REFRESH was used, it's not like
there is 3rd possible behavior.

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Get stuck when dropping a subscription during synchronizing table

2017-05-24 Thread Petr Jelinek
Hi,

I finally had time to properly analyze this, and turns out we've been
all just trying to fix symptoms and the actual problems.

All the locking works just fine the way it is in master. The issue with
deadlock with apply comes from the wrong handling of the SIGTERM in the
apply (we didn't set InterruptPending). I changed the SIGTERM handler in
patch 0001 just to die which is actually the correct behavior for apply
workers. I also moved the connection cleanup code to the
before_shmem_exit callback (similar to walreceiver) and now that part
works correctly.

The issue with orphaned sync workers is actually two separate issues.
First, due to thinko we always searched for sync worker in
wait_for_sync_status_change instead of searching for opposite worker as
was the intention (i.e. sync worker should search for apply and apply
should search for sync). Thats fixed by 0002. And second, we didn't
accept any invalidation messages until the whole sync process finished
(because it flattens all the remote transactions in the single one) so
sync worker didn't learn about subscription changes/drop until it has
finished, which I now fixed in 0003.

There is still outstanding issue that sync worker will keep running
inside the long COPY because the invalidation messages are also not
processed until it finishes but all the original issues reported here
disappear for me with the attached patches applied.

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


0003-Receive-invalidation-messages-correctly-in-tablesync.patch
Description: binary/octet-stream


0002-Make-tablesync-worker-exit-when-apply-dies-while-it-.patch
Description: binary/octet-stream


0001-Fix-signal-handling-in-logical-workers.patch
Description: binary/octet-stream

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] walsender & parallelism

2017-05-23 Thread Petr Jelinek
On 23/05/17 19:45, Andres Freund wrote:
> 
> 
> On May 23, 2017 1:42:41 PM EDT, Petr Jelinek <petr.jeli...@2ndquadrant.com> 
> wrote:
>> Hi,
>>
>> so this didn't really move anywhere AFAICS, do we think the approach
>> I've chosen is good or do we want to do something else here?
> 
> Can you add it to the open items list?
> 

Done

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Why does logical replication launcher set application_name?

2017-05-23 Thread Petr Jelinek
On 20/04/17 21:33, Peter Eisentraut wrote:
> On 4/18/17 13:18, Tom Lane wrote:
>> I think you're thinking about it wrong.  To my mind the issue is that
>> there should be some generic way to determine that a bgworker process
>> is or is not laboring on behalf of an identifiable user.  It's great
>> that we can tell which user it is when there is one, but clearly some
>> bgworkers will be providing general services that aren't associated with
>> a single user.  So it should be possible to set the userID to zero or
>> some such when the bgworker is one that isn't associated with a
>> particular user.  Maybe the owning user needs to become an additional
>> parameter passed in struct BackgroundWorker.
> 
> I think this is probably a problem particular to the logical replication
> launcher.  Other background workers either do work as a particular user,
> as you say, or don't touch the database at all.  So a localized hack or
> a simple hide-the-user flag might suffice for now.
> 

But that still leaves the application_name issue. My proposal in general
would be to add boolean that indicates that the worker is not using
specific user (this can be easily set in
InitializeSessionUserIdStandalone()) and will work for multiple things.

About application_name, perhaps we should just add bgw_type or bgw_group
and show it as worker_type in activity and that's it?

I think this should be open item btw so I'll add it.

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] walsender & parallelism

2017-05-23 Thread Petr Jelinek
Hi,

so this didn't really move anywhere AFAICS, do we think the approach
I've chosen is good or do we want to do something else here?

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Multiple table synchronizations are processed serially

2017-05-19 Thread Petr Jelinek
On 19/05/17 21:47, Peter Eisentraut wrote:
> On 5/19/17 01:01, Masahiko Sawada wrote:
>> Seems all four table sync workers are launched at the almost same
>> time, but three workers of them get stuck in idle transaction state
>> when creating replication slot. That is these waiting workers cannot
>> proceed its work until first connected table sync worker finishes. ps
>> command shows the followings.
> 
> Creating a replication slot waits for all transactions to finish.  So if
> one of those transactions is a table copy of another subscription, it
> has to wait for that.
> 

Well IMHO this concrete example is partially effect of the snapshort
builder fixes we did with Andres. Before those fixes the ondisk snapshot
may have been used in this situation. Makes me think if we want to mark
the exported snapshot as containing all transactions and using it if
that's the case.

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Get stuck when dropping a subscription during synchronizing table

2017-05-19 Thread Petr Jelinek
On 18/05/17 16:16, Robert Haas wrote:
> On Wed, May 17, 2017 at 6:58 AM, Masahiko Sawada <sawada.m...@gmail.com> 
> wrote:
>> I think the above changes can solve this issue but It seems to me that
>> holding AccessExclusiveLock on pg_subscription by DROP SUBSCRIPTION
>> until commit could lead another deadlock problem in the future. So I'd
>> to contrive ways to reduce lock level somehow if possible. For
>> example, if we change the apply launcher so that it gets the
>> subscription list only when pg_subscription gets invalid, apply
>> launcher cannot try to launch the apply worker being stopped. We
>> invalidate pg_subscription at commit of DROP SUBSCRIPTION and the
>> apply launcher can get new subscription list which doesn't include the
>> entry we removed. That way we can reduce lock level to
>> ShareUpdateExclusiveLock and solve this issue.
>> Also in your patch, we need to change DROP SUBSCRIPTION as well to
>> resolve another case I encountered, where DROP SUBSCRIPTION waits for
>> apply worker while holding a tuple lock on pg_subscription_rel and the
>> apply worker waits for same tuple on pg_subscription_rel in
>> SetSubscriptionRelState().
> 
> I don't really understand the issue being discussed here in any
> detail, but as a general point I'd say that it might be more
> productive to make the locks finer-grained rather than struggling to
> reduce the lock level.  For example, instead of locking all of
> pg_subscription, use LockSharedObject() to lock the individual
> subscription, still with AccessExclusiveLock.  That means that other
> accesses to that subscription also need to take a lock so that you
> actually get a conflict when there should be one, but that should be
> doable.  I expect that trying to manage locking conflicts using only
> catalog-wide locks is a doomed strategy.

We do LockSharedObject() but it's rather useless the way it's done now
as no other access locks it. We can't block all other accesses however,
the workers need to be able to access the catalog during clean shutdown
in some situations. What we need is to block starting of new workers for
that subscription so only those code paths would need to block. So I
think we might want to do both finer-grained locking and decreasing lock
level.

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] snapbuild woes

2017-05-14 Thread Petr Jelinek
On 13/05/17 22:23, Andres Freund wrote:
> On 2017-05-12 10:57:55 +0200, Petr Jelinek wrote:
>> Hmm, well it helps but actually now that we don't track individual
>> running transactions anymore it got much less effective (my version of
>> 0005 does as well).
>>
>> The example workload I test with is:
>> session 1: open transaction, do a write, keep it open
>> session 2: pgbench  -M simple -N -c 10 -P 1 -T 5
>> session 3: run CREATE_REPLICATION_SLOT LOGICAL in walsender
>> session 2: pgbench  -M simple -N -c 10 -P 1 -T 20
>> session 1: commit
>>
>> And wait for session 3 to finish slot creation, takes about 20 mins on
>> my laptop without patches, minute and half with your patches for 0004
>> and 0005 (or with your 0004 and my 0005) and about 2s with my original
>> 0004 and 0005.
> 
> Is that with assertions enabled or not?  With assertions all the time
> post patches seems to be spent in some Asserts in reorderbuffer.c,
> without it takes less than a second for me here.
>

Right you are, I always forget to switch that off before benchmarks.

> I'm appylying these now.

Cool. Just for posterity, this also fixes the issue number 3 as the
switch to consistent state is done purely based on xl_running_xacts and
not decoded commits/aborts.

So all done here, thanks!

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] snapbuild woes

2017-05-12 Thread Petr Jelinek
On 12/05/17 10:57, Petr Jelinek wrote:
> On 12/05/17 03:31, Andres Freund wrote:
>> On 2017-05-11 14:54:26 -0700, Andres Freund wrote:
>>> On 2017-05-11 14:51:55 -0700,  wrote:
>>>> On 2017-05-08 00:10:12 -0700, Andres Freund wrote:
>>>>> I plan to commit the next pending patch after the back branch releases
>>>>> are cut - it's an invasive fix and the issue doesn't cause corruption
>>>>> "just" slow slot creation. So it seems better to wait for a few days,
>>>>> rather than hurry it into the release.
>>>>
>>>> Now that that's done, here's an updated version of that patch.  Note the
>>>> new logic to trigger xl_running_xact's to be logged at the right spot.
>>>> Works well in my testing.
>>>>
>>>> I plan to commit this fairly soon, unless somebody wants a bit more time
>>>> to look into it.
>>>>
>>>> Unless somebody protests, I'd like to slightly revise how the on-disk
>>>> snapshots are stored on master - given the issues this bug/commit showed
>>>> with the current method - but I can see one could argue that that should
>>>> rather be done next release.
>>>
>>> As usual I forgot my attachement.
>>
>> This patch also seems to offer a way to do your 0005 in, possibly, more
>> efficient manner.  We don't ever need to assume a transaction is
>> timetravelling anymore.  Could you check whether the attached, hasty,
>> patch resolves the performance problems you measured?  Also, do you have
>> a "reference" workload for that?
>>
> 
> Hmm, well it helps but actually now that we don't track individual
> running transactions anymore it got much less effective (my version of
> 0005 does as well).
> 
> The example workload I test with is:
> session 1: open transaction, do a write, keep it open
> session 2: pgbench  -M simple -N -c 10 -P 1 -T 5
> session 3: run CREATE_REPLICATION_SLOT LOGICAL in walsender
> session 2: pgbench  -M simple -N -c 10 -P 1 -T 20
> session 1: commit
> 
> And wait for session 3 to finish slot creation, takes about 20 mins on
> my laptop without patches, minute and half with your patches for 0004
> and 0005 (or with your 0004 and my 0005) and about 2s with my original
> 0004 and 0005.
> 
> What makes it slow is the constant qsorting IIRC.
> 

Btw I still think that we should pursue the approach you proposed. I
think we can deal with the qsorting in some other ways (ordered insert
perhaps?) later. What you propose fixes the correctness, makes
performance less awful in the above workload and also makes the
snapbuild bit easier to read.

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] If subscription to foreign table valid ?

2017-05-12 Thread Petr Jelinek
On 12/05/17 15:55, Neha Khatri wrote:
> On 5/11/17, Petr Jelinek <petr.jeli...@2ndquadrant.com> wrote:
>> Hi,
>>
>> On 11/05/17 14:25, tushar wrote:
>>> Hi,
>>>
>>> I observed that -we cannot publish "foreign table" in Publication
>>>
>>> but same thing is not true for Subscription
>>>
>>> postgres=# create foreign table t (n int) server db1_server options
>>> (table_name 't');
>>> CREATE FOREIGN TABLE
>>> postgres=# alter subscription sub refresh publication ;
>>> NOTICE:  added subscription for table public.t
>>> ALTER SUBSCRIPTION
>>>
>>> Is this an expected behavior ?   if we cannot publish then how  can we
>>> add subscription for it.
> 
> The above foreign table subscription succeeds only if the publisher
> has published a same named normal table i.e.
>   create table t (n int);
>   CREATE PUBLICATION mypub FOR TABLE t;
> 
> I think in current implemetation of LogicalRep. it is users
> responsibility to match the table definition at publisher and
> subscriber. Subscriber can not determine by itself what publisher has
> defined. This usecase does not align with this assumption.
> 
> 
>> However, the foreign tables indeed can't be subscribed.
> 
> I suspect that a user would want to subcribe a foreign table in real world.
> 
>> I think it does make sense to add check for this into CREATE/ALTER
>> SUBSCRIBER though so that user is informed immediately about the mistake
>> rather than by errors in the logs later.
> 
> Yes, system should prohibit such operation though.
> I tried to write to a patch to achieve this. It disalows subscription
> of relation other than RELKIND_RELATION.
> Attached is the patch. Comments?
> 

I sent my version of patch in parallel. I think we don't need to do the
relation open like you did, all the info is in syscache.

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] logical replication syntax (was DROP SUBSCRIPTION, query cancellations and slot handling)

2017-05-12 Thread Petr Jelinek
On 12/05/17 15:02, Peter Eisentraut wrote:
> On 5/9/17 11:43, Petr Jelinek wrote:
>> Here is rebased version of the other patch (the interface rework). I
>> also fixed the tab completion there.
> 
> Committed.
> 
> One small thing I changed, to make it look more like CREATE/ALTER TABLE,
> is that the CREATE commands use WITH (...) but the ALTER commands use
> SET (...).
> 

Makes sense, thanks!

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Time based lag tracking for logical replication

2017-05-12 Thread Petr Jelinek
On 12/05/17 15:09, Neha Khatri wrote:
> On Fri, May 12, 2017 at 8:19 PM, Simon Riggs <si...@2ndquadrant.com> wrote:
>>
>> On 11 May 2017 at 18:29, Simon Riggs <si...@2ndquadrant.com> wrote:
>>> On 11 May 2017 at 18:13, Andres Freund <and...@anarazel.de> wrote:
>>>
>>>>> New patch, v3.
>>>>>
>>>>> Applying in 90 minutes, barring objections.
>>>>
>>>> Could you please wait till tomorrow?  I've bigger pending fixes for 
>>>> related code pending/being tested that I plan to push today.  I'd also 
>>>> like to take a look before...
>>>
>>> Sure.
>>
>> The changes I've added are very minor, so I'm not expecting debate.
>> The main part of the patch is the same as Petr posted 19days ago.
>>
>> I'm travelling now, so after waiting till tomorrow as you requested I
>> have committed the patch.
>>
> 
> Prior to this commit CREATE SUBSCRIPTION used to work smoothly.
> 
> After this commit 024711bb544645c8b1061e9f02b261e2e336981d I get
> following error while executing CREATE SUBSCRIPTION:
> 
> CREATE SUBSCRIPTION sub1 CONNECTION 'dbname=postgres host=localhost
> user=neha port=5432' PUBLICATION mypub;
> NOTICE:  synchronized table states
> ERROR:  could not create replication slot "sub1": ERROR:  could not
> load library "/home/neha/postgres/PGCurrentInstall/lib/pgoutput.so":
> /home/neha/postgres/PGCurrentInstall/lib/pgoutput.so: undefined
> symbol: OutputPluginUpdateProgress
> 

Hmm, that sounds like partial rebuild/install (old postgres binary with
new pgoutput one).

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] If subscription to foreign table valid ?

2017-05-12 Thread Petr Jelinek
On 11/05/17 15:43, Petr Jelinek wrote:
> Hi,
> 
> On 11/05/17 14:25, tushar wrote:
>> Hi,
>>
>> I observed that -we cannot publish "foreign table" in Publication
>>
>> postgres=# create foreign table t (n int) server db1_server options
>> (table_name 't1');
>> CREATE FOREIGN TABLE
>>
>> postgres=# create publication pub for table t;
>> ERROR:  "t" is not a table
>> DETAIL:  Only tables can be added to publications.
>> postgres=#
>>
>> but same thing is not true for Subscription
>>
>> postgres=# create foreign table t (n int) server db1_server options
>> (table_name 't');
>> CREATE FOREIGN TABLE
>> postgres=# alter subscription sub refresh publication ;
>> NOTICE:  added subscription for table public.t
>> ALTER SUBSCRIPTION
>>
>> Is this an expected behavior ?   if we cannot publish then how  can we 
>> add subscription for it.
>>
> 
> Thanks for report. What you can publish and what you can subscribe is
> not necessarily same (we can write to relations which we can't capture
> from wal, for example unlogged table can't be published but can be
> subscribed).
> 
> However, the foreign tables indeed can't be subscribed. I originally
> planned to have foreign tables allowed on subscriber but it turned out
> to be more complex to implement than I had anticipated do I ripped the
> code for that from the original patch.
> 
> We do check for this, but only during replication which we have to do
> because the fact that relation 't' was foreign table during ALTER
> SUBSCRIPTION does not mean that it won't be something else half hour later.
> 
> I think it does make sense to add check for this into CREATE/ALTER
> SUBSCRIBER though so that user is informed immediately about the mistake
> rather than by errors in the logs later.
> 
> I'll look into writing patch for this. I don't think it's beta blocker
> though.
> 

So I moved the relkind check to single function and call it from all the
necessary places. See the attached

I now wonder if we should do some other checks as well (columns etc).

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


Check-relkind-of-tables-in-CREATE-ALTER-SUBSCRIPTION.patch
Description: binary/octet-stream

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] snapbuild woes

2017-05-12 Thread Petr Jelinek
On 12/05/17 03:31, Andres Freund wrote:
> On 2017-05-11 14:54:26 -0700, Andres Freund wrote:
>> On 2017-05-11 14:51:55 -0700,  wrote:
>>> On 2017-05-08 00:10:12 -0700, Andres Freund wrote:
>>>> I plan to commit the next pending patch after the back branch releases
>>>> are cut - it's an invasive fix and the issue doesn't cause corruption
>>>> "just" slow slot creation. So it seems better to wait for a few days,
>>>> rather than hurry it into the release.
>>>
>>> Now that that's done, here's an updated version of that patch.  Note the
>>> new logic to trigger xl_running_xact's to be logged at the right spot.
>>> Works well in my testing.
>>>
>>> I plan to commit this fairly soon, unless somebody wants a bit more time
>>> to look into it.
>>>
>>> Unless somebody protests, I'd like to slightly revise how the on-disk
>>> snapshots are stored on master - given the issues this bug/commit showed
>>> with the current method - but I can see one could argue that that should
>>> rather be done next release.
>>
>> As usual I forgot my attachement.
> 
> This patch also seems to offer a way to do your 0005 in, possibly, more
> efficient manner.  We don't ever need to assume a transaction is
> timetravelling anymore.  Could you check whether the attached, hasty,
> patch resolves the performance problems you measured?  Also, do you have
> a "reference" workload for that?
> 

Hmm, well it helps but actually now that we don't track individual
running transactions anymore it got much less effective (my version of
0005 does as well).

The example workload I test with is:
session 1: open transaction, do a write, keep it open
session 2: pgbench  -M simple -N -c 10 -P 1 -T 5
session 3: run CREATE_REPLICATION_SLOT LOGICAL in walsender
session 2: pgbench  -M simple -N -c 10 -P 1 -T 20
session 1: commit

And wait for session 3 to finish slot creation, takes about 20 mins on
my laptop without patches, minute and half with your patches for 0004
and 0005 (or with your 0004 and my 0005) and about 2s with my original
0004 and 0005.

What makes it slow is the constant qsorting IIRC.

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] snapbuild woes

2017-05-11 Thread Petr Jelinek
On 11/05/17 16:33, Stas Kelvich wrote:
> 
>> On 10 May 2017, at 11:43, Petr Jelinek <petr.jeli...@2ndquadrant.com> wrote:
>>
>> On 09/05/17 22:11, Erik Rijkers wrote:
>>> On 2017-05-09 21:00, Petr Jelinek wrote:
>>>> On 09/05/17 19:54, Erik Rijkers wrote:
>>>>> On 2017-05-09 11:50, Petr Jelinek wrote:
>>>>>
>>>>
>>>> Ah okay, so this is same issue that's reported by both Masahiko Sawada
>>>> [1] and Jeff Janes [2].
>>>>
>>>> [1]
>>>> https://www.postgresql.org/message-id/CAD21AoBYpyqTSw%2B%3DES%2BxXtRGMPKh%3DpKiqjNxZKnNUae0pSt9bg%40mail.gmail.com
>>>>
>>>> [2]
>>>> https://www.postgresql.org/message-id/flat/CAMkU%3D1xUJKs%3D2etq2K7bmbY51Q7g853HLxJ7qEB2Snog9oRvDw%40mail.gmail.com
>>>>
>>>
>>> I don't understand why you come to that conclusion: both Masahiko Sawada
>>> and Jeff Janes have a DROP SUBSCRIPTION in the mix; my cases haven't. 
>>> Isn't that a real difference?
>>>
>>
> 
> Just noted another one issue/feature with snapshot builder — when logical 
> decoding is in progress
> and vacuum full command is being issued quite a big amount of files appears 
> in pg_logical/mappings
> and reside there till the checkpoint. Also having consequent vacuum full’s 
> before checkpoint yields even
> more files.
> 
> Having two pgbench-filled instances with logical replication between them:
> 
> for i in {1..100}; do psql -c 'vacuum full' && ls -la pg_logical/mappings | 
> wc -l; done;
> VACUUM
>   73
> VACUUM
>  454
> VACUUM
> 1146
> VACUUM
> 2149
> VACUUM
> 3463
> VACUUM
> 5088
> <...>
> VACUUM
>44708
> <…> // checkpoint happens somewhere here
> VACUUM
>20921
> WARNING:  could not truncate file "base/16384/61773": Too many open files in 
> system
> ERROR:  could not fsync file 
> "pg_logical/mappings/map-4000-4df-0_A4EA29F8-5aa5-5ae6": Too many open files 
> in system
> 
> 
> I’m not sure whether this is boils down to some of the previous issues 
> mentioned here or not, so posting
> here as observation.
> 

This has nothing to do with snapshot builder so this is not the thread
for it. See the comment section near the bottom of
src/backend/access/heap/rewriteheap.c for explanation of why it does
what it does.

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] If subscription to foreign table valid ?

2017-05-11 Thread Petr Jelinek
Hi,

On 11/05/17 14:25, tushar wrote:
> Hi,
> 
> I observed that -we cannot publish "foreign table" in Publication
> 
> postgres=# create foreign table t (n int) server db1_server options
> (table_name 't1');
> CREATE FOREIGN TABLE
> 
> postgres=# create publication pub for table t;
> ERROR:  "t" is not a table
> DETAIL:  Only tables can be added to publications.
> postgres=#
> 
> but same thing is not true for Subscription
> 
> postgres=# create foreign table t (n int) server db1_server options
> (table_name 't');
> CREATE FOREIGN TABLE
> postgres=# alter subscription sub refresh publication ;
> NOTICE:  added subscription for table public.t
> ALTER SUBSCRIPTION
> 
> Is this an expected behavior ?   if we cannot publish then how  can we 
> add subscription for it.
> 

Thanks for report. What you can publish and what you can subscribe is
not necessarily same (we can write to relations which we can't capture
from wal, for example unlogged table can't be published but can be
subscribed).

However, the foreign tables indeed can't be subscribed. I originally
planned to have foreign tables allowed on subscriber but it turned out
to be more complex to implement than I had anticipated do I ripped the
code for that from the original patch.

We do check for this, but only during replication which we have to do
because the fact that relation 't' was foreign table during ALTER
SUBSCRIPTION does not mean that it won't be something else half hour later.

I think it does make sense to add check for this into CREATE/ALTER
SUBSCRIBER though so that user is informed immediately about the mistake
rather than by errors in the logs later.

I'll look into writing patch for this. I don't think it's beta blocker
though.

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Time based lag tracking for logical replication

2017-05-11 Thread Petr Jelinek
On 11/05/17 15:01, Simon Riggs wrote:
> On 11 May 2017 at 08:32, Noah Misch <n...@leadboat.com> wrote:
>> On Sun, Apr 23, 2017 at 01:10:32AM +0200, Petr Jelinek wrote:
>>> The time based lag tracking commit [1] added interface for logging
>>> progress of replication so that we can report lag as time interval
>>> instead of just bytes. But the patch didn't contain patch for the
>>> builtin logical replication.
>>>
>>> So I wrote something that implements this.
>>
>> This is listed as a PostgreSQL 10 open item, but the above makes it sound 
>> like
>> a feature to consider for v11, not a defect in v10.  Why is this an open 
>> item?
> 
> It's an open item because at the time of the Lag Tracker commit it was
> believed to be a single line of code that needed to be added to
> Logical Replication to make it work with the Lag Tracker
> functionality. It didn't make sense for me to add it while the LR code
> was still being changed, even though we had code to do that.
> 
> Petr's new patch is slightly longer and needed review and some minor
> code to add pacing delay.
> 
> My own delay in responding has been because of illness. You'll note
> that I'd missed response on at least one other mail from you.
> Apologies for that, it has set me back some way but I'm better now and
> have caught up with other matters. Petr nudged me to look at this
> thread again yesterday, so I had been looking at this over last few
> days.
> 
> Attached patch is Petr's patch, slightly rebased with added pacing
> delay, similar to that used by HSFeedback.
> 

This looks reasonable. I would perhaps change:
> +   /*
> +* Track lag no more than once per 
> WALSND_LOGICAL_LAG_TRACK_INTERVAL_MS
> +*/

to something like this for extra clarity:
> +   /*
> +    * Track lag no more than once per 
> WALSND_LOGICAL_LAG_TRACK_INTERVAL_MS
> +* to avoid flooding the lag tracker on busy servers.
> +*/

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Re: logical replication syntax (was DROP SUBSCRIPTION, query cancellations and slot handling)

2017-05-11 Thread Petr Jelinek
On 11/05/17 09:27, Noah Misch wrote:
> On Mon, May 08, 2017 at 05:01:00PM -0400, Peter Eisentraut wrote:
>> On 5/7/17 04:21, Noah Misch wrote:
>>> The above-described topic is currently a PostgreSQL 10 open item.  Peter,
>>> since you committed the patch believed to have created it, you own this open
>>> item.  If some other commit is more relevant or if this does not belong as a
>>> v10 open item, please let us know.  Otherwise, please observe the policy on
>>> open item ownership[1] and send a status update within three calendar days 
>>> of
>>> this message.  Include a date for your subsequent status update.  Testers 
>>> may
>>> discover new open items at any time, and I want to plan to get them all 
>>> fixed
>>> well in advance of shipping v10.  Consequently, I will appreciate your 
>>> efforts
>>> toward speedy resolution.  Thanks.
>>
>> I think we have a workable patch, but it needs some rebasing.  I hope to
>> have this sorted by Wednesday.
> 
> This PostgreSQL 10 open item is past due for your status update.  Kindly send
> a status update within 24 hours, and include a date for your subsequent status
> update.  Refer to the policy on open item ownership:
> https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com
> 

The patch Peter mentioned was committed.

There is however another open item associated with this thread for which
there is another patch as well.

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Get stuck when dropping a subscription during synchronizing table

2017-05-11 Thread Petr Jelinek
On 11/05/17 10:10, Masahiko Sawada wrote:
> On Thu, May 11, 2017 at 4:06 PM, Michael Paquier
> <michael.paqu...@gmail.com> wrote:
>> On Wed, May 10, 2017 at 11:57 AM, Masahiko Sawada <sawada.m...@gmail.com> 
>> wrote:
>>> Barring any objections, I'll add these two issues to open item.
>>
>> It seems to me that those open items have not been added yet to the
>> list. If I am following correctly, they could be defined as follows:
>> - Dropping subscription may stuck if done during tablesync.
>> -- Analyze deadlock issues with DROP SUBSCRIPTION and apply worker process.

I think the solution to this is to reintroduce the LWLock that was
removed and replaced with the exclusive lock on catalog [1]. I am afraid
that correct way of handling this is to do both LWLock and catalog lock
(first LWLock under which we kill the workers and then catalog lock so
that something that prevents launcher from restarting them is held till
the end of transaction).

>> -- Avoid orphaned tablesync worker if apply worker exits before
>> changing its status.
> 

The behavior question I have about this is if sync workers should die
when apply worker dies (ie they are tied to apply worker) or if they
should be tied to the subscription.

I guess taking down all the sync workers when apply worker has exited is
easier to solve. Of course it means that if apply worker restarts in
middle of table synchronization, the table synchronization will have to
start from scratch. That being said, in normal operation apply worker
should only exit/restart if subscription has changed or has been
dropped/disabled and I think sync workers want to exit/restart in that
situation as well.

So for example having shmem detach hook for an apply worker (or reusing
the existing one) that searches for all the other workers for same
subscription and shuts them down as well sounds like solution to this.

[1]
https://www.postgresql.org/message-id/cahgqgwhpi8ky-yanffe0sgmhktsycqltnkx07bw9s7-rn17...@mail.gmail.com

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Should pg_current_wal_location() become pg_current_wal_lsn()

2017-05-10 Thread Petr Jelinek
On 10/05/17 21:22, Tom Lane wrote:
> Robert Haas <robertmh...@gmail.com> writes:
>> On Wed, May 10, 2017 at 1:13 PM, Tom Lane <t...@sss.pgh.pa.us> wrote:
>>> In terms of the alternatives I listed previously, it seems like
>>> nobody liked alternatives #3, #4, or #5, leaving us with #1 (do
>>> nothing) or #2 (apply this patch).  By my count, Peter is the
>>> only one in favor of doing nothing, and is outvoted.  I'll push
>>> the patch later today if I don't hear additional comments.
> 
>> For the record, I also voted for doing nothing.
> 
> Hm, well, anybody else want to vote?
> 

I am for standardizing (although I don't have preference of location vs
lsn).

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Function to move the position of a replication slot

2017-05-10 Thread Petr Jelinek
On 10/05/17 22:17, Dmitry Dolgov wrote:
>> On 4 May 2017 at 20:05, Magnus Hagander <mag...@hagander.net
> <mailto:mag...@hagander.net>> wrote:
>>
>> PFA a patch that adds a new function, pg_move_replication_slot, that
> makes it
>> possible to move the location of a replication slot without actually
>> consuming all the WAL on it.
>  
> Just a few questions just a few questions out of curiosity:
> 
> * does it make sense to create a few tests for this function in
>   `contrib/test_decoding` (as shown in attachment)?
> 

As Craig said this will not work correctly for logical slots so it
should throw error on those at the moment (at least until we write
something that works, but that's far more complex than this patch).

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] 'nocopy data' option is set in SUBSCRIPTION but still data is getting migrated

2017-05-10 Thread Petr Jelinek
On 10/05/17 15:27, tushar wrote:
> Hi,
> 
> Please refer this scenario -where 'nocopy data' option is set in
> SUBSCRIPTION but still data is getting migrated
> 
> Publication - (X)
> create table t(n int);
> insert into t values (generate_series(1,99));
> create publication pub for table  t;
> 
> Subscription (Y)
> create table t(n int);
> CREATE SUBSCRIPTION sub CONNECTION 'dbname=postgres host=localhost
> port=5000 user=centos password=a' PUBLICATION pub WITH (copy
> data,SYNCHRONOUS_COMMIT=on);
> select count(*) from t;  ->showing 99 rows
> alter subscription sub refresh publication with (nocopy data);
> restart the server (Y)
> 
> X - insert more records into table 't'
> Y - check the row count , rows have been migrated from X .
> 
> Is it the right behavior in this case where nocopy data option is set ?
> 

Yes, (no)copy data only affects existing data at the time of running the
command, any additional data are always replicated.

The "alter subscription sub refresh publication" does nothing unless you
added/removed tables to/from publication.

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] [BUGS] Concurrent ALTER SEQUENCE RESTART Regression

2017-05-10 Thread Petr Jelinek
On 10/05/17 07:09, Peter Eisentraut wrote:
> On 5/7/17 19:43, Andres Freund wrote:
>> 3. Keep the catalog, make ALTER properly transactional, blocking
>>concurrent nextval()s. This resolves the issue that nextval() can't
>>see the changed definition of the sequence.
> 
> This was the intended choice.
> 
> [...]
> 
> 5. nextval() disregarding uncommitted ALTER SEQUENCE changes.  In
><PG10, it would read the uncommitted metadata and observe it.
>Currently, it goes ahead even if there is an uncommitted ALTER
>SEQUENCE pending that would prohibit what it's about to do (e.g.,
>MAXVALUE change).
> 
>I think the correct fix is to have nextval() and ALTER SEQUENCE use
>sensible lock levels so that they block each other.  Since
>nextval() currently uses AccessShareLock, the suggestion was for
>ALTER SEQUENCE to therefore use AccessExclusiveLock.  But I think a
>better idea would be for nextval() to use RowExclusiveLock
>(analogous to UPDATE) and ALTER SEQUENCE to use
>ShareRowExclusiveLock, which would also satisfy issue #1.
> 

When I proposed this upstream, Andres raised concern about performance
of nextval() if we do this, did you try to run any benchmark on this?

Looking at the changes to open_share_lock(), I wonder if we need to have
lock level as parameter so that lastval() is not blocked.

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] snapbuild woes

2017-05-10 Thread Petr Jelinek
On 09/05/17 22:11, Erik Rijkers wrote:
> On 2017-05-09 21:00, Petr Jelinek wrote:
>> On 09/05/17 19:54, Erik Rijkers wrote:
>>> On 2017-05-09 11:50, Petr Jelinek wrote:
>>>
>>
>> Ah okay, so this is same issue that's reported by both Masahiko Sawada
>> [1] and Jeff Janes [2].
>>
>> [1]
>> https://www.postgresql.org/message-id/CAD21AoBYpyqTSw%2B%3DES%2BxXtRGMPKh%3DpKiqjNxZKnNUae0pSt9bg%40mail.gmail.com
>>
>> [2]
>> https://www.postgresql.org/message-id/flat/CAMkU%3D1xUJKs%3D2etq2K7bmbY51Q7g853HLxJ7qEB2Snog9oRvDw%40mail.gmail.com
>>
> 
> I don't understand why you come to that conclusion: both Masahiko Sawada
> and Jeff Janes have a DROP SUBSCRIPTION in the mix; my cases haven't. 
> Isn't that a real difference?
> 

Because I only see the sync workers running in the output you shown, the
bug described in those threads can as one of side effects cause the sync
workers to wait forever for the apply that was killed, crashed, etc, in
the right moment, which I guess is what happened during your failed test
(there should be some info in the log about apply exiting).

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] snapbuild woes

2017-05-09 Thread Petr Jelinek
On 09/05/17 19:54, Erik Rijkers wrote:
> On 2017-05-09 11:50, Petr Jelinek wrote:
> 
>> I rebased the above mentioned patch to apply to the patches Andres sent,
>> if you could try to add it on top of what you have and check if it still
>> fails, that would be helpful.
> 
> It still fails.
> 
> With these patches
> 
> - 0001-WIP-Fix-off-by-one-around-GetLastImportantRecPtr.patch+
> - 2-WIP-Possibly-more-robust-snapbuild-approach.patch +
> - fix-statistics-reporting-in-logical-replication-work.patch +
> - Skip-unnecessary-snapshot-builds.patch
> 
> built again on top of 44c528810a1 ( so I had to add the
> 'fix-statistics-rep*' patch because without it I immediately got that
> Assertion failure again ).
> 
> As always most runs succeed (especially on this large 192GB 16-core
> server).
> 
> But attached is an output file of a number of runs of my
> pgbench_derail2.sh test.
> 
> Overal result:
> 
> -- out_20170509_1635.txt
>   3 -- pgbench -c 64 -j 8 -T 900 -P 180 -n   --  scale 25
>   2 -- All is well.
>   1 -- Not good, but breaking out of wait (21 times no change)
> 
> I broke it off after iteration 4, so 5 never ran, and
> iteration 1 failed due to a mistake in the harness (somethind stupid I
> did) - not interesting.
> 
> iteration 2 succeeds. (eventually has 'replica ok')
> 
> iteration 3 succeeds. (eventually has 'replica ok')
> 
> iteration 4 fails.
>   Just after 'alter subscription sub1 enable' I caught (as is usual)
> pg_stat_replication.state as 'catchup'. So far so good.
>   After the 15-minute pgbench run pg_stat_replication has only 2
> 'startup' lines (and none 'catchup' or 'streaming'):
> 
>  port | pg_stat_replication |  pid   | wal | replay_loc | diff |
> ?column? |  state  |   app   | sync_state
>  6972 | pg_stat_replication | 108349 | 19/8FBCC248 ||  |
>  | startup | derail2 | async
>  6972 | pg_stat_replication | 108351 | 19/8FBCC248 ||  |
>  | startup | derail2 | async
> 
> (that's from:
>select $port1 as port,'pg_stat_replication' as pg_stat_replication, pid
>  , pg_current_wal_location() wal, replay_location replay_loc,
> pg_current_wal_location() - replay_location as diff
>  , pg_current_wal_location() <= replay_location
>  , state, application_name as app, sync_state
>from pg_stat_replication
> )
> 
> This remains in this state for as long as my test-programs lets it
> (i.e., 20 x 30s, or something like that, and then the loop is exited);
> in the ouput file it says: 'Not good, but breaking out of wait'
> 
> Below is the accompanying ps (with the 2 'deranged senders' as Jeff
> Janes would surely call them):
> 
> 
> UID PID   PPID  C STIME TTY  STAT   TIME CMD
> rijkers  107147  1  0 17:11 pts/35   S+ 0:00
> /var/data1/pg_stuff/pg_installations/pgsql.logical_replication2/bin/postgres
> -D /var/data1/pg_stuff/pg_installations
> rijkers  107149 107147  0 17:11 ?Ss 0:00  \_ postgres:
> logger process
> rijkers  107299 107147  0 17:11 ?Ss 0:01  \_ postgres:
> checkpointer process
> rijkers  107300 107147  0 17:11 ?Ss 0:00  \_ postgres:
> writer process
> rijkers  107301 107147  0 17:11 ?Ss 0:00  \_ postgres: wal
> writer process
> rijkers  107302 107147  0 17:11 ?Ss 0:00  \_ postgres:
> autovacuum launcher process
> rijkers  107303 107147  0 17:11 ?Ss 0:00  \_ postgres: stats
> collector process
> rijkers  107304 107147  0 17:11 ?Ss 0:00  \_ postgres:
> bgworker: logical replication launcher
> rijkers  108348 107147  0 17:12 ?Ss 0:01  \_ postgres:
> bgworker: logical replication worker for subscription 70310 sync 70293
> rijkers  108350 107147  0 17:12 ?Ss 0:00  \_ postgres:
> bgworker: logical replication worker for subscription 70310 sync 70298
> rijkers  107145  1  0 17:11 pts/35   S+ 0:02
> /var/data1/pg_stuff/pg_installations/pgsql.logical_replication/bin/postgres
> -D /var/data1/pg_stuff/pg_installations
> rijkers  107151 107145  0 17:11 ?Ss 0:00  \_ postgres:
> logger process
> rijkers  107160 107145  0 17:11 ?Ss 0:08  \_ postgres:
> checkpointer process
> rijkers  107161 107145  0 17:11 ?Ss 0:07  \_ postgres:
> writer process
> rijkers  107162 107145  0 17:11 ?Ss 0:02  \_ postgres: wal
> writer process
> rijkers  107163 107145  0 17:11 ?Ss 0:00  \_ postgres:
> autovacuum launcher process
> rijkers  107164 107145  0 17:11 ?Ss 0:02  \_ postgres: stats
> collector process
> rijkers  107165 107145  0 17:11 ?Ss 0:00  \_ postgres:
> bgworke

Re: [HACKERS] logical replication deranged sender

2017-05-09 Thread Petr Jelinek
On 09/05/17 19:13, Jeff Janes wrote:
> On Tue, May 9, 2017 at 9:18 AM, Petr Jelinek
> <petr.jeli...@2ndquadrant.com <mailto:petr.jeli...@2ndquadrant.com>> wrote:
> 
> On 08/05/17 13:47, Petr Jelinek wrote:
> > On 08/05/17 01:17, Jeff Janes wrote:
> >> After dropping a subscription, it says it succeeded and that it dropped
> >> the slot on the publisher.
> >>
> >> But the publisher still has the slot, and a full-tilt process described
> >> by ps as
> >>
> >> postgres: wal sender process jjanes [local] idle in transaction
> >>
> >> Strace shows that this process is doing nothing but opening, reading,
> >> lseek, and closing from pg_wal, and calling sbrk.  It never sends 
> anything.
> >>
> >> This is not how it should work, correct?
> >>
> >
> > No, and I don't see how this happens though, we only report success if
> > the publisher side said that DROP_REPLICATION_SLOT succeeded. So far I
> > don't see anything in source that would explain this. I will need to
> > reproduce it first to see what's happening (wasn't able to do that yet,
> > but it might just need more time since you say it does no happen 
> always).
> >
> 
> Hm I wonder are there any workers left on subscriber when this happens?
> 
> 
> Yes.  using ps, I get this:
> 
> postgres: bgworker: logical replication worker for subscription 16408
> sync 16391
> postgres: bgworker: logical replication worker for subscription 16408
> sync 16388
> 
> They seem to be permanently blocked on a socket to read from the publisher.
> 
> On the publisher side, I think it is very slowly assembling a snapshot. 
> It seems to be adding one xid at a time, and then re-sorting the entire
> list.  Over and over.
> 

Okay, then it's the same issue Masahiko Sawada reported in nearby
thread, or at least has same cause.

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] logical replication deranged sender

2017-05-09 Thread Petr Jelinek
On 08/05/17 13:47, Petr Jelinek wrote:
> On 08/05/17 01:17, Jeff Janes wrote:
>> After dropping a subscription, it says it succeeded and that it dropped
>> the slot on the publisher.
>>
>> But the publisher still has the slot, and a full-tilt process described
>> by ps as 
>>
>> postgres: wal sender process jjanes [local] idle in transaction
>>
>> Strace shows that this process is doing nothing but opening, reading,
>> lseek, and closing from pg_wal, and calling sbrk.  It never sends anything.
>>
>> This is not how it should work, correct?
>>
> 
> No, and I don't see how this happens though, we only report success if
> the publisher side said that DROP_REPLICATION_SLOT succeeded. So far I
> don't see anything in source that would explain this. I will need to
> reproduce it first to see what's happening (wasn't able to do that yet,
> but it might just need more time since you say it does no happen always).
> 

Hm I wonder are there any workers left on subscriber when this happens?

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] logical replication syntax (was DROP SUBSCRIPTION, query cancellations and slot handling)

2017-05-09 Thread Petr Jelinek
On 09/05/17 16:28, Peter Eisentraut wrote:
> On 5/9/17 04:39, Petr Jelinek wrote:
>>>> What we want to simulate instead is an "auto" dependency of the slot on
>>>> the subscription.  So you can drop the slot separately (subject to other
>>>> restrictions), and it is dropped automatically when the subscription is
>>>> dropped.  To avoid that, you can disassociate the slot from the
>>>> subscription, which you have implemented.
>>>>
>>>> I think we can therefore do without RESTRICT/CASCADE here.  If a slot is
>>>> associated with the subscription, it should be there when we drop the
>>>> subscription.  Otherwise, the user has to disassociate the slot and take
>>>> care of it manually.  So just keep the "cascade" behavior.
>>>>
>>>> Similarly, I wouldn't check first whether the slot exists.  If the
>>>> subscription is associated with the slot, it should be there.
>>>
>>> Here is your patch amended for that.
>>
>> I am fine with this mechanism as well.
> 
> Committed.
> 
> I also wrote a bit of documentation about slot handling for
> subscriptions, covering some of what was discussed in this thread.
> 

Great, thanks.

Here is rebased version of the other patch (the interface rework). I
also fixed the tab completion there.

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


Rework-the-options-for-logical-replication-v2.patch
Description: binary/octet-stream

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] logical replication syntax (was DROP SUBSCRIPTION, query cancellations and slot handling)

2017-05-09 Thread Petr Jelinek
On 09/05/17 11:44, Masahiko Sawada wrote:
> On Tue, May 9, 2017 at 5:57 PM, Petr Jelinek
> <petr.jeli...@2ndquadrant.com> wrote:
>> On 09/05/17 10:51, Masahiko Sawada wrote:
>>> On Tue, May 9, 2017 at 5:39 PM, Petr Jelinek
>>> <petr.jeli...@2ndquadrant.com> wrote:
>>>> On 09/05/17 07:07, Peter Eisentraut wrote:
>>>>> On 5/8/17 23:23, Peter Eisentraut wrote:
>>>>>> The way this uses RESTRICT and CASCADE appears to be backwards from its
>>>>>> usual meaning.  Normally, CASCADE when dropping an object that is still
>>>>>> used by others will cause those other objects to be dropped.  The
>>>>>> equivalent here would be DROP REPLICATION SLOT + CASCADE would drop the
>>>>>> subscription.
>>>>>>
>>>>>> What we want to simulate instead is an "auto" dependency of the slot on
>>>>>> the subscription.  So you can drop the slot separately (subject to other
>>>>>> restrictions), and it is dropped automatically when the subscription is
>>>>>> dropped.  To avoid that, you can disassociate the slot from the
>>>>>> subscription, which you have implemented.
>>>>>>
>>>>>> I think we can therefore do without RESTRICT/CASCADE here.  If a slot is
>>>>>> associated with the subscription, it should be there when we drop the
>>>>>> subscription.  Otherwise, the user has to disassociate the slot and take
>>>>>> care of it manually.  So just keep the "cascade" behavior.
>>>>>>
>>>>>> Similarly, I wouldn't check first whether the slot exists.  If the
>>>>>> subscription is associated with the slot, it should be there.
>>>
>>> IIUC in this design, for example if we mistakenly create a
>>> subscription without creating replication slot and corresponding
>>> replication slot doesn't exist on publisher, we cannot drop
>>> subscription until we create corresponding replication slot manually.
>>> Isn't it become a problem for user?
>>>
>>
>> We can, that's why the NONE value for slot name was added by the patch
>> so that subscription can be made "slot-less".
> 
> Yeah, but since we can still create subscription with only NOCREATE
> SLOT option (option name will be changed but still exists), if we do
> that then non-NULL value is stored into subslotname and the
> subscription is enable. We can drop such subscription after disabled
> it and after set its slot name to NONE. But I think it's still not
> good for user..
> 

Well that's why I originally had the options directly as part of DROP
SUBSCRIPTION. But if you read the discussion in this thread, that's not
something we should be doing. There is no sensible way of mapping the
nodrop behavior to the only allowed options of RESTRICT and CASCADE so
there is not much we can do other than the ALTER.

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] snapbuild woes

2017-05-09 Thread Petr Jelinek
On 09/05/17 10:59, Erik Rijkers wrote:
> On 2017-05-09 10:50, Petr Jelinek wrote:
>> On 09/05/17 00:03, Erik Rijkers wrote:
>>> On 2017-05-05 02:00, Andres Freund wrote:
>>>>
>>>> Could you have a look?
>>>
>>> Running tests with these three patches:
>>>
>>>> 0001-WIP-Fix-off-by-one-around-GetLastImportantRecPtr.patch+
>>>> 0002-WIP-Possibly-more-robust-snapbuild-approach.patch +
>>>> fix-statistics-reporting-in-logical-replication-work.patch
>>> (on top of 44c528810)
>>>
>>> I test by 15-minute pgbench runs while there is a logical replication
>>> connection. Primary and replica are on the same machine.
>>>
>>> I have seen errors on 3 different machines (where error means: at least
>>> 1 of the 4 pgbench tables is not md5-equal). It seems better, faster
>>> machines yield less errors.
>>>
>>> Normally I see in pg_stat_replication (on master) one process in state
>>> 'streaming'.
>>>
>>>  pid  | wal | replay_loc  |   diff   |   state   |   app   |
>>> sync_state
>>> 16495 | 11/EDBC | 11/EA3FEEE8 | 58462488 | streaming | derail2 |
>>> async
>>>
>>> Often there are another two processes in pg_stat_replication that remain
>>> in state 'startup'.
>>>
>>> In the failing sessions the 'streaming'-state process is missing; in
>>> failing sessions there are only the two processes that are and remain in
>>> 'startup'.
>>
>> Hmm, startup is the state where slot creation is happening. I wonder if
>> it's just taking long time to create snapshot because of the 5th issue
>> which is not yet fixed (and the original patch will not apply on top of
>> this change). Alternatively there is a bug in this patch.
>>
>> Did you see high CPU usage during the test when there were those
>> "startup" state walsenders?
>>
> 
> I haven't noticed but I didn't pay attention to that particularly.
> 
> I'll try to get some CPU-info logged...
> 

I rebased the above mentioned patch to apply to the patches Andres sent,
if you could try to add it on top of what you have and check if it still
fails, that would be helpful.

Thanks!

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


Skip-unnecessary-snapshot-builds.patch
Description: binary/octet-stream

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] logical replication syntax (was DROP SUBSCRIPTION, query cancellations and slot handling)

2017-05-09 Thread Petr Jelinek
On 09/05/17 10:51, Masahiko Sawada wrote:
> On Tue, May 9, 2017 at 5:39 PM, Petr Jelinek
> <petr.jeli...@2ndquadrant.com> wrote:
>> On 09/05/17 07:07, Peter Eisentraut wrote:
>>> On 5/8/17 23:23, Peter Eisentraut wrote:
>>>> The way this uses RESTRICT and CASCADE appears to be backwards from its
>>>> usual meaning.  Normally, CASCADE when dropping an object that is still
>>>> used by others will cause those other objects to be dropped.  The
>>>> equivalent here would be DROP REPLICATION SLOT + CASCADE would drop the
>>>> subscription.
>>>>
>>>> What we want to simulate instead is an "auto" dependency of the slot on
>>>> the subscription.  So you can drop the slot separately (subject to other
>>>> restrictions), and it is dropped automatically when the subscription is
>>>> dropped.  To avoid that, you can disassociate the slot from the
>>>> subscription, which you have implemented.
>>>>
>>>> I think we can therefore do without RESTRICT/CASCADE here.  If a slot is
>>>> associated with the subscription, it should be there when we drop the
>>>> subscription.  Otherwise, the user has to disassociate the slot and take
>>>> care of it manually.  So just keep the "cascade" behavior.
>>>>
>>>> Similarly, I wouldn't check first whether the slot exists.  If the
>>>> subscription is associated with the slot, it should be there.
> 
> IIUC in this design, for example if we mistakenly create a
> subscription without creating replication slot and corresponding
> replication slot doesn't exist on publisher, we cannot drop
> subscription until we create corresponding replication slot manually.
> Isn't it become a problem for user?
> 

We can, that's why the NONE value for slot name was added by the patch
so that subscription can be made "slot-less". The change of
RESTRICT/CASCADE behavior that Peter made is just about whether we
refuse to drop subscription by default when there is slot associated
with or if we just go straight to dropping the slot.

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] SUBSCRIPTIONS and pg_upgrade

2017-05-09 Thread Petr Jelinek
On 09/05/17 07:24, Michael Paquier wrote:
> On Sat, May 6, 2017 at 12:01 AM, Peter Eisentraut
> <peter.eisentr...@2ndquadrant.com> wrote:
>> On 5/2/17 21:44, Noah Misch wrote:
>>>> I wonder if we should have an --no-subscriptions option, now that they
>>>> are dumped by default, just like we have --no-blobs, --no-owner,
>>>> --no-password, --no-privileges, --no-acl, --no-tablespaces, and
>>>> --no-security-labels.  It seems like there is probably a fairly large
>>>> use case for excluding subscriptions even if you have sufficient
>>>> permissions to dump them.
> 
> Reading the thread for the first time, I am +1 on that. It feels more
> natural to have subscriptions by default when taking a dump, and it is
> useful as well to be able to override the default so as they are not
> included.

Yes I agree. There was also talk of generalizing the --no-* stuff so we
could filter arbitrary objects which is something I would prefer as
solution, but at this point that's potential PG11 feature not PG10 one.

> 
> On Tue, May 9, 2017 at 1:26 AM, Peter Eisentraut
> <peter.eisentr...@2ndquadrant.com> wrote:
>> On 5/6/17 14:50, Noah Misch wrote:
>>>> I consider this item low priority and don't plan to work on it before
>>>> all the other open items under logical replication are addressed.
>>>>
>>>> (Here, working on it would include thinking further about whether it is
>>>> necessary at all or what alternatives might look like.)
>>>
>>> That's informative, but it's not a valid status update.  This PostgreSQL 10
>>> open item is past due for your status update.  Kindly send a valid status
>>> update within 24 hours.  Refer to the policy on open item ownership:
>>> https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com
>>
>> Fair enough.  I have set myself a reminder to report back on May 30.
> 
> I think that it would be nice to fix that even before beta, so
> attached is a patch to add --no-subscriptions to pg_dump, pg_dumpall
> and pg_restore.
> 

Looks okay to me, it's simple enough patch.

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] snapbuild woes

2017-05-09 Thread Petr Jelinek
On 09/05/17 00:03, Erik Rijkers wrote:
> On 2017-05-05 02:00, Andres Freund wrote:
>>
>> Could you have a look?
> 
> Running tests with these three patches:
> 
>> 0001-WIP-Fix-off-by-one-around-GetLastImportantRecPtr.patch+
>> 0002-WIP-Possibly-more-robust-snapbuild-approach.patch +
>> fix-statistics-reporting-in-logical-replication-work.patch
> (on top of 44c528810)
> 
> I test by 15-minute pgbench runs while there is a logical replication
> connection. Primary and replica are on the same machine.
> 
> I have seen errors on 3 different machines (where error means: at least
> 1 of the 4 pgbench tables is not md5-equal). It seems better, faster
> machines yield less errors.
> 
> Normally I see in pg_stat_replication (on master) one process in state
> 'streaming'.
> 
>  pid  | wal | replay_loc  |   diff   |   state   |   app   |
> sync_state
> 16495 | 11/EDBC | 11/EA3FEEE8 | 58462488 | streaming | derail2 | async
> 
> Often there are another two processes in pg_stat_replication that remain
> in state 'startup'.
> 
> In the failing sessions the 'streaming'-state process is missing; in
> failing sessions there are only the two processes that are and remain in
> 'startup'.

Hmm, startup is the state where slot creation is happening. I wonder if
it's just taking long time to create snapshot because of the 5th issue
which is not yet fixed (and the original patch will not apply on top of
this change). Alternatively there is a bug in this patch.

Did you see high CPU usage during the test when there were those
"startup" state walsenders?

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] logical replication syntax (was DROP SUBSCRIPTION, query cancellations and slot handling)

2017-05-09 Thread Petr Jelinek
On 09/05/17 07:07, Peter Eisentraut wrote:
> On 5/8/17 23:23, Peter Eisentraut wrote:
>> The way this uses RESTRICT and CASCADE appears to be backwards from its
>> usual meaning.  Normally, CASCADE when dropping an object that is still
>> used by others will cause those other objects to be dropped.  The
>> equivalent here would be DROP REPLICATION SLOT + CASCADE would drop the
>> subscription.
>>
>> What we want to simulate instead is an "auto" dependency of the slot on
>> the subscription.  So you can drop the slot separately (subject to other
>> restrictions), and it is dropped automatically when the subscription is
>> dropped.  To avoid that, you can disassociate the slot from the
>> subscription, which you have implemented.
>>
>> I think we can therefore do without RESTRICT/CASCADE here.  If a slot is
>> associated with the subscription, it should be there when we drop the
>> subscription.  Otherwise, the user has to disassociate the slot and take
>> care of it manually.  So just keep the "cascade" behavior.
>>
>> Similarly, I wouldn't check first whether the slot exists.  If the
>> subscription is associated with the slot, it should be there.
> 
> Here is your patch amended for that.
> 

I am fine with this mechanism as well.

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] logical replication syntax (was DROP SUBSCRIPTION, query cancellations and slot handling)

2017-05-08 Thread Petr Jelinek
On 08/05/17 22:55, Peter Eisentraut wrote:
> On 5/5/17 13:01, Petr Jelinek wrote:
>> What do you think of attached. I actually did add RESTRICT/CASCADE, in a
>> way that if there is slot RESTRICT will refuse to drop subscription and
>> CASCADE will try to drop it. Still all errors.
>>
>> The new way to not drop slot is to set slot_name to NONE which is new
>> value that I invented (inspiration from OWNED BY sequences) which
>> basically disassociates the subscription from slot.
>>
>> It's slightly less automatic this way but perhaps that's not a bad
>> thing, at least nobody will drop slots by mistake while we still make
>> sure that slots are not left over without anybody noticing.
> 
> I think this is OK.  Could you send a version of this patch based on
> master please?
> 

Here it is.

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


Remove-the-NODROP-SLOT-option-from-DROP-SUBSCRIPTION-0508.patch
Description: binary/octet-stream

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Logical replication - TRAP: FailedAssertion in pgstat.c

2017-05-08 Thread Petr Jelinek
On 08/05/17 18:12, Peter Eisentraut wrote:
> On 5/5/17 07:13, Petr Jelinek wrote:
>> Yes that's different thing that we've been discussing a bit in snapbuild
>> woes thread.
>>
>>> the 'TRAP: FailedAssertion in pgstat.c' anymore.
>>>
>>> (If there is any other configuration of patches worth testing please let
>>> me know)
>>
>> Thanks, so the patch works.
> 
> Committed, with s/xact_started/started_tx/, to match nearby code.
> 

Thanks!

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Logical replication - TRAP: FailedAssertion in pgstat.c

2017-05-08 Thread Petr Jelinek
On 08/05/17 17:52, Masahiko Sawada wrote:
> On Fri, May 5, 2017 at 8:13 PM, Petr Jelinek
> <petr.jeli...@2ndquadrant.com> wrote:
>> On 03/05/17 13:23, Erik Rijkers wrote:
>>> On 2017-05-03 08:17, Petr Jelinek wrote:
>>>> On 02/05/17 20:43, Robert Haas wrote:
>>>>> On Thu, Apr 20, 2017 at 2:58 PM, Peter Eisentraut
>>>
>>>>>> code path that calls CommitTransactionCommand() should have one, no?
>>>>>
>>>>> Is there anything left to be committed here?
>>>>>
>>>>
>>>> Afaics the fix was not committed. Peter wanted more comprehensive fix
>>>> which didn't happen. I think something like attached should do the job.
>>>
>>> I'm running my pgbench-over-logical-replication test in chunk of 15
>>> minutes, wth different pgbench -c (num clients) and -s (scale) values.
>>>
>>> With this patch (and nothing else)  on top of master (8f8b9be51fd7 to be
>>> precise):
>>>
>>>> fix-statistics-reporting-in-logical-replication-work.patch
>>>
>>> logical replication is still often failing (as expected, I suppose; it
>>> seems because of "inital snapshot too large") but indeed I do not see
>>
>> Yes that's different thing that we've been discussing a bit in snapbuild
>> woes thread.
>>
>>> the 'TRAP: FailedAssertion in pgstat.c' anymore.
>>>
>>> (If there is any other configuration of patches worth testing please let
>>> me know)
>>>
>>
>> Thanks, so the patch works.
>>
> 
> I think that we should commit the local transaction that did initial
> data copy, and then report stat as well. Currently table sync worker
> doesn't commit the local transaction in LogicalRepSyncTableStart
> (maybe until apply commit record?) if its status is changed to
> SUBREL_STATE_CATCHUP. That's why the table sync worker issues
> assertion failure.
> 

That would fix the assert as well yes, but it would also mean that if
the worker crashed between the initial copy and the end of catchup there
would be no way to restart it without manual intervention from user
since the synchronization position would be lost. Hence the fix I
proposed which does it differently and has the whole sync in a single
transaction.

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] logical replication deranged sender

2017-05-08 Thread Petr Jelinek
On 08/05/17 01:17, Jeff Janes wrote:
> After dropping a subscription, it says it succeeded and that it dropped
> the slot on the publisher.
> 
> But the publisher still has the slot, and a full-tilt process described
> by ps as 
> 
> postgres: wal sender process jjanes [local] idle in transaction
> 
> Strace shows that this process is doing nothing but opening, reading,
> lseek, and closing from pg_wal, and calling sbrk.  It never sends anything.
> 
> This is not how it should work, correct?
> 

No, and I don't see how this happens though, we only report success if
the publisher side said that DROP_REPLICATION_SLOT succeeded. So far I
don't see anything in source that would explain this. I will need to
reproduce it first to see what's happening (wasn't able to do that yet,
but it might just need more time since you say it does no happen always).

> 
> The subscribing server's logs shows:
> 
> 21270  2017-05-07 16:04:16.517 PDT LOG:  process 21270 still waiting for
> AccessShareLock on relation 6100 of database 0 after 1000.051 ms
> 21270  2017-05-07 16:04:16.517 PDT DETAIL:  Process holding the lock:
> 25493. Wait queue: 21270.
> 21270  2017-05-07 16:04:16.520 PDT LOG:  process 21270 acquired
> AccessShareLock on relation 6100 of database 0 after 1003.608 ms
> 25493 DROP SUBSCRIPTION 2017-05-07 16:04:16.520 PDT LOG:  duration:
> 1005.176 ms CPU: user: 0.00 s, system: 0.00 s, elapsed: 1.00 s  
> statement: drop subscription foobar ;
> 

Yeah that's normal because DropSubscription holds exclusive lock on
pg_subscription. I am guessing 21270 is the logical replication launcher?

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Get stuck when dropping a subscription during synchronizing table

2017-05-08 Thread Petr Jelinek
On 08/05/17 11:27, Masahiko Sawada wrote:
> Hi,
> 
> I encountered a situation where DROP SUBSCRIPTION got stuck when
> initial table sync is in progress. In my environment, I created
> several tables with some data on publisher. I created subscription on
> subscriber and drop subscription immediately after that. It doesn't
> always happen but I often encountered it on my environment.
> 
> ps -x command shows the following.
> 
>  96796 ?Ss 0:00 postgres: masahiko postgres [local] DROP
> SUBSCRIPTION
>  96801 ?Ts 0:00 postgres: bgworker: logical replication
> worker for subscription 40993waiting
>  96805 ?Ss 0:07 postgres: bgworker: logical replication
> worker for subscription 40993 sync 16418
>  96806 ?Ss 0:01 postgres: wal sender process masahiko [local] idle
>  96807 ?Ss 0:00 postgres: bgworker: logical replication
> worker for subscription 40993 sync 16421
>  96808 ?Ss 0:00 postgres: wal sender process masahiko [local] idle
> 
> The DROP SUBSCRIPTION process (pid 96796) is waiting for the apply
> worker process (pid 96801) to stop while holding a lock on
> pg_subscription_rel. On the other hand the apply worker is waiting for
> acquiring a tuple lock on pg_subscription_rel needed for heap_update.
> Also table sync workers (pid 96805 and 96807) are waiting for the
> apply worker process to change their status.
> 

Looks like we should kill apply before dropping dependencies.

> Also, even when DROP SUBSCRIPTION is done successfully, the table sync
> worker can be orphaned because I guess that the apply worker can exit
> before change status of table sync worker.

Well the tablesync worker should stop itself if the subscription got
removed, but of course again the dependencies are an issue, so we should
probably kill those explicitly as well.

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Draft release notes for next week's back-branch releases

2017-05-08 Thread Petr Jelinek
On 06/05/17 19:38, Tom Lane wrote:
> Petr Jelinek <petr.jeli...@2ndquadrant.com> writes:
>> On 06/05/17 19:15, Tom Lane wrote:
>>> (Or, wait a minute.  That documentation only applies to v10, but we
>>> need to be writing this relnote for 9.6 users.  What terminology should
>>> we be using anyway?)
> 
>> Yeah we need to somehow mention that it only affects 3rd party tools
>> using logical decoding.
> 
>> "The initial snapshot created for a logical decoding slot was
>> potentially incorrect.  This could allow the 3rd party tools using
>> the logical decoding to copy incomplete existing(?) data.  This was
>> more likely to happen if the source server was busy at the time of
>> slot creation, or if two slots were created concurrently."
> 
>>> Also, do we need to recommend that people not trust any logical replicas
>>> at this point, but recreate them after installing the update?
> 
>> Yes, but only if there was preexisting data *and* there was concurrent
>> activity on the server when the "replication" was setup.
> 
> OK, I can work with this.  Thanks for the help!
> 

Great, thanks.

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Draft release notes for next week's back-branch releases

2017-05-06 Thread Petr Jelinek
On 06/05/17 19:15, Tom Lane wrote:
> Petr Jelinek <petr.jeli...@2ndquadrant.com> writes:
>> On 06/05/17 18:16, Tom Lane wrote:
>>> Hmm, I'm hoping for something more user-oriented.  Is the corruption
>>> time-limited?  What's an "exported snapshot" anyway, is it the same
>>> thing as pg_export_snapshot(), and if so what's that got to do with
>>> logical replication?
> 
>> Okay to explain what's happening. When you create logical replication
>> slot via walsender, it exports snapshot (like the one exported by
>> pg_export_snapshot() indeed) which corresponds to exact point in time
>> where the decoding will start for the slot. You can import this snapshot
>> to another backend and use it to copy any existing data before starting
>> the replication. The bugs cause that these snapshots would be corrupted
>> and you'd have inconsistent data (some tuples missing). One of them
>> would export snapshot that's only valid for system catalogs but not user
>> tables (the ondisk snapshot, this needs at least one preexisting slot).
>> The other would not guarantee that tuples needed by the snapshot weren't
>> removed by vacuum of HOT pruning (the window being only the time it took
>> to generate the snapshot).
> 
> OK, that's better.  But I'm still not really seeing a reason to treat
> these as two separate items for release-note purposes: I don't think users

Sure my main issue was that the text combined the info from two commits
in a way that made the statement no longer correct.

> will care.  Now that I've read section 31.4, I'd suggest that we phrase
> the release notes in the terms it uses.  How about saying something like
> "The initial snapshot created for a logical replication slot was
> incorrect.  This could allow the apply process to copy incomplete or
> inconsistent data.  This was more likely to happen if the source server
> was busy at the time of slot creation, or if two slots were created
> concurrently" ?
> 
> (Or, wait a minute.  That documentation only applies to v10, but we
> need to be writing this relnote for 9.6 users.  What terminology should
> we be using anyway?)

Yeah we need to somehow mention that it only affects 3rd party tools
using logical decoding.

"The initial snapshot created for a logical decoding slot was
potentially incorrect.  This could allow the 3rd party tools using
the logical decoding to copy incomplete existing(?) data.  This was
more likely to happen if the source server was busy at the time of
slot creation, or if two slots were created concurrently."

> 
> Also, do we need to recommend that people not trust any logical replicas
> at this point, but recreate them after installing the update?
> 

Yes, but only if there was preexisting data *and* there was concurrent
activity on the server when the "replication" was setup.

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Draft release notes for next week's back-branch releases

2017-05-06 Thread Petr Jelinek
On 06/05/17 18:16, Tom Lane wrote:
> Petr Jelinek <petr.jeli...@2ndquadrant.com> writes:
>> On 06/05/17 17:25, Tom Lane wrote:
>>> OK, can you suggest better wording?
> 
>> Something like the attached (it requires some polishing of English
>> probably).
> 
> Hmm, I'm hoping for something more user-oriented.  Is the corruption
> time-limited?  What's an "exported snapshot" anyway, is it the same
> thing as pg_export_snapshot(), and if so what's that got to do with
> logical replication?
> 

Well user does not care unless they use something like pglogical, or
bottledwater, or wal2json, etc.

Okay to explain what's happening. When you create logical replication
slot via walsender, it exports snapshot (like the one exported by
pg_export_snapshot() indeed) which corresponds to exact point in time
where the decoding will start for the slot. You can import this snapshot
to another backend and use it to copy any existing data before starting
the replication. The bugs cause that these snapshots would be corrupted
and you'd have inconsistent data (some tuples missing). One of them
would export snapshot that's only valid for system catalogs but not user
tables (the ondisk snapshot, this needs at least one preexisting slot).
The other would not guarantee that tuples needed by the snapshot weren't
removed by vacuum of HOT pruning (the window being only the time it took
to generate the snapshot).

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Draft release notes for next week's back-branch releases

2017-05-06 Thread Petr Jelinek
On 06/05/17 17:25, Tom Lane wrote:
> Petr Jelinek <petr.jeli...@2ndquadrant.com> writes:
>> Hmm, the 2bef06d51 and 56e19d938 are fixes for different bugs, we can
>> keep them together since result of both is corrupted snapshot, but the
>> description can't just mangle pieces of text from different commits
>> together like this as that's misleading.
> 
> OK, can you suggest better wording?
> 

Something like the attached (it requires some polishing of English
probably).

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services
diff --git a/doc/src/sgml/release-9.6.sgml b/doc/src/sgml/release-9.6.sgml
index cdff084..195cbb1 100644
--- a/doc/src/sgml/release-9.6.sgml
+++ b/doc/src/sgml/release-9.6.sgml
@@ -92,17 +92,24 @@ Branch: REL9_5_STABLE [54270d7eb] 2017-04-27 15:29:43 -0700
 Branch: REL9_4_STABLE [b6ecf26cc] 2017-04-27 15:29:52 -0700
 -->
  
-  Fix possibly-corrupt initial snapshot during logical decoding
-  (Petr Jelinek, Andres Freund)
+  Fix two bugs resulting in possibly-corrupt initial exported snapshot
+  during logical decoding (Petr Jelinek, Andres Freund)
  
 
  
   If a logical decoding replication slot was created while another slot
-  already exists, it was initialized with a potentially-corrupted
-  snapshot, allowing wrong data to be returned during decoding.
-  The time window during which this snapshot continued to be used
-  depended on how busy the server was; under low load it would be hard
-  to see any problem.
+  already exists, it would exported a potentially-corrupted snapshot,
+  allowing wrong data to be returned when such snapshot was imported to
+  another backend.
+ 
+
+ 
+  Logical decoding normally preserves all required historical catalog
+  tuples, it didn't guarantee that the non-catalog tuples also
+  were preserved for exported snapshot. This could have resulted in
+  concurrent VACUUM or HOT pruning to remove tuples needed by the
+  snapshot. The time window for this was very small and the problem
+  required server to be very busy during slot creation.
  
 
 

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Draft release notes for next week's back-branch releases

2017-05-06 Thread Petr Jelinek
On 06/05/17 01:37, Tom Lane wrote:
> I've written $SUBJECT ... you can find them at
> 
> https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=54dbd4dc78b045ffcc046b9a43681770c3992dd4
> 

Hmm, the 2bef06d51 and 56e19d938 are fixes for different bugs, we can
keep them together since result of both is corrupted snapshot, but the
description can't just mangle pieces of text from different commits
together like this as that's misleading.

The 2bef06d51 is about not preserving global xmin correctly, window for
that is indeed small. But the 56e19d938 was about using ondisk snapshot
(which only contain transactions changing catalogs) as full snapshots,
it's more about creating multiple logical replication slots while
concurrent writes happen.

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Not getting error if ALTER SUBSCRIPTION syntax is wrong.

2017-05-06 Thread Petr Jelinek
On 05/05/17 19:51, Petr Jelinek wrote:
> On 05/05/17 14:40, tushar wrote:
>> Hi,
>>
>> While testing 'logical replication' against v10 , i encountered one
>> issue where data stop migrating after ALTER PUBLICATION.
>>
>> X Server
>> \\ Make sure wal_level is set to logical in postgresql.conf file
>> \\create table/Insert 1 row -> create table test(n int); insert into t
>> values (1);
>> \\create publication for all -> create publication pub for ALL TABLES ;
>>
>>
>> Y server
>>
>> \\ Make sure wal_level is set to logical in postgresql.conf file
>> \\create table -> create table test(n int);
>>
>> \\create Subscription
>>
>> CREATE SUBSCRIPTION sub CONNECTION 'host=localhost dbname=postgres
>> port=5432 ' PUBLICATION pub;
>>
>> [...]
>>
>> I think probably syntax of alter subscription is not correct but
>> surprisingly it is not throwing an error.
>>
> 
> Syntax of ALTER command is correct, syntax of the connection string is
> not, you are probably getting errors in log from the replication worker.
> 
> We could check validity of the connection string though to complain
> immediately like we do in CREATE.
> 

The attached does exactly that.

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


Check-connection-info-in-ALTER-SUBSCRIPTION.patch
Description: binary/octet-stream

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Not getting error if ALTER SUBSCRIPTION syntax is wrong.

2017-05-05 Thread Petr Jelinek
On 05/05/17 14:40, tushar wrote:
> Hi,
> 
> While testing 'logical replication' against v10 , i encountered one
> issue where data stop migrating after ALTER PUBLICATION.
> 
> X Server
> \\ Make sure wal_level is set to logical in postgresql.conf file
> \\create table/Insert 1 row -> create table test(n int); insert into t
> values (1);
> \\create publication for all -> create publication pub for ALL TABLES ;
> 
> 
> Y server
> 
> \\ Make sure wal_level is set to logical in postgresql.conf file
> \\create table -> create table test(n int);
> 
> \\create Subscription
> 
> CREATE SUBSCRIPTION sub CONNECTION 'host=localhost dbname=postgres
> port=5432 ' PUBLICATION pub;
> 
> postgres=# select * from test;
>  n
> ---
>  1
> (1 row)
> 
> \\Alter subscription
> postgres=# alter subscription sub connection 'host=localhost
> dbname=postgres PUBLICATION pub';
> ALTER SUBSCRIPTION
> 
> X server
> postgres=# insert into test values (1);
> INSERT 0 1
> postgres=# select * from test;
>  n
> ---
>  1
>  1
> (2 rows)
> 
> Y server
> postgres=# select * from test;
>  n
> ---
>  1
> (1 row)
> 
> I think probably syntax of alter subscription is not correct but
> surprisingly it is not throwing an error.
> 

Syntax of ALTER command is correct, syntax of the connection string is
not, you are probably getting errors in log from the replication worker.

We could check validity of the connection string though to complain
immediately like we do in CREATE.

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] snapbuild woes

2017-05-05 Thread Petr Jelinek
On 05/05/17 18:18, Andres Freund wrote:
> On 2017-05-05 13:53:16 +0200, Petr Jelinek wrote:
>> On 05/05/17 02:42, Andres Freund wrote:
>>> On 2017-05-04 17:00:04 -0700, Andres Freund wrote:
>>>> Attached is a prototype patch for that.
>>>
>>
>> I am not sure I understand the ABI comment for started_collection_at.
>> What's the ABI issue? The struct is private to snapbuild.c module. Or
>> you want to store it in the ondisk snapshot as well?
> 
> It's stored on-disk already :(

Hmm okay, well then I guess we'll have to store it somewhere inside
running, we should not normally care about that since we only load
CONSISTENT snapshots where running don't matter anymore. Alternatively
we could bump the SNAPBUILD_VERSION but that would make it impossible to
downgrade minor version which is bad (I guess we'll want to do that for
master, but not back-branches).

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] logical replication syntax (was DROP SUBSCRIPTION, query cancellations and slot handling)

2017-05-05 Thread Petr Jelinek
On 02/05/17 15:31, Tom Lane wrote:
> Petr Jelinek <petr.jeli...@2ndquadrant.com> writes:
>> Let me expand, if we don't drop the slot by default when dropping
>> subscription, we'll have a lot of users with dead slots laying around
>> holding back WAL/catalog_xmin, that's really bad. If we do drop by
>> default like now, we need option to not do that, neither RESTRICT, nor
>> CASCADE fit that.
> 
> I'm not sure which part of "you can't have an option in DROP" isn't
> clear to you.  Whatever the default behavior is always has to work,
> because that is what's going to happen during DROP OWNED BY, or
> DROP DATABASE.  If you want more than one behavior during DROP,
> you need to drive that off something represented as a persistent
> property of the object, not off an option in the DROP command.
> 
> I agree that RESTRICT/CASCADE don't fit this, unless the model
> is that the slot is always owned by the subscription, which might
> be too restrictive.
> 

What do you think of attached. I actually did add RESTRICT/CASCADE, in a
way that if there is slot RESTRICT will refuse to drop subscription and
CASCADE will try to drop it. Still all errors.

The new way to not drop slot is to set slot_name to NONE which is new
value that I invented (inspiration from OWNED BY sequences) which
basically disassociates the subscription from slot.

It's slightly less automatic this way but perhaps that's not a bad
thing, at least nobody will drop slots by mistake while we still make
sure that slots are not left over without anybody noticing.

Note that this would need catalog version bump as it removes NOT NULL
constraint from subslotname.

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


Remove-the-NODROP-SLOT-option-from-DROP-SUBSCRIPTION.patch
Description: binary/octet-stream

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] compiler warning with VS 2017

2017-05-05 Thread Petr Jelinek
On 05/05/17 16:56, Tom Lane wrote:
> Petr Jelinek <petr.jeli...@2ndquadrant.com> writes:
>> On 05/05/17 06:50, Tom Lane wrote:
>>> Actually, looking around a bit there, it's not even clear why
>>> we should be booby-trapping the value of an unchanged column in
>>> the first place.  So I'd say that not only is the code dubious
>>> but the comment is inadequate too.
> 
>> Hmm, as far as I can recollect this is just leftover debugging code that
>> was intended to help ensure that we are checking the "changed"
>> everywhere we are supposed to (since I changed handling of these
>> structured quite a bit during development). Should be changed to NULL,
>> that's what we usually do in this type of situation.
> 
> So the comment should be something like "if the column is unchanged,
> we should not attempt to access its value beyond this point.  To
> help catch any such attempts, set the string to NULL" ?
> 

Yes that sounds about right. We don't get any data for unchanged TOAST
columns (that's limitation of logical decoding) so we better not touch them.

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] [patch] Build pgoutput with MSVC

2017-05-05 Thread Petr Jelinek
On 05/05/17 12:10, Magnus Hagander wrote:
> 
> On Fri, May 5, 2017 at 11:58 AM, Michael Paquier
> <michael.paqu...@gmail.com <mailto:michael.paqu...@gmail.com>> wrote:
> 
> On Fri, May 5, 2017 at 6:10 PM, MauMau <maumau...@gmail.com
> <mailto:maumau...@gmail.com>> wrote:
> > The pgoutput is not built with MSVC.  The attached patch fixes this.
> > I confirmed that a few INSERTs were replicated correctly.
> >
> > Should I add this matter in the PostgreSQL 10 Open Items page?
> 
> Yes, with Peter as committer and Petr as owner.
> 
> +my $pgoutput = $solution->AddProject(
> +'pgoutput', 'dll', '',
> +'src/backend/replication/pgoutput');
> +$pgoutput->AddReference($postgres);
> Yup, that's correct.
> 
> You have forgotten to update clean.bat, which should clean up
> pgoutput.dll.
> 
> 
> If that's all that's required, I'll just go ahead and commit it right
> away, including the clean.bat.
> 
> I think the problem with clean.bat isn't cleaning up pgoutput.dll --
> that one goes in a different directory. But it does need to clean up the
> win32ver.rc file that gets dropped there automaticaly.
> 
> The attached patch itself seems broken (it has some sort of byte order
> marker at the beginning, but removing that still breaks with "patch
> unexpectedly ends in middle of line patch:  Only garbage was found
> in the patch input.". But I can just copy/paste it manually :)
> 

Thanks for fixing this, I admit I have no idea how our windows build
system works.

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] PG 10 release notes

2017-05-05 Thread Petr Jelinek
On 05/05/17 02:46, Bruce Momjian wrote:
> On Thu, May  4, 2017 at 05:09:40PM -0700, Andres Freund wrote:
>>>> I would not in any way refer to logical decoding as being only a proof
>>>> of concept, even before logical replication.
>>>
>>> The community ships a reliable logical _encoding_, and a test logical
>>> _decoding_.
>>
>> Yes, so what?  What you said is "I didn't think logical decoding was
>> really more than a proof-of-concept until now", which is plainly wrong,
>> given I know a significant number of users using it in production.  Some
>> of them are well known & large enterprises, and it's used to enable
>> critical things.
> 
> I am getting tired of saying this.  When I am writing the release notes,
> I am trying to figure out how it affects our shipped code, and the only
> "decoding" I know of is test_decoding.  My message was this:

I actually think the main misunderstanding here comes from the
test_decoding name interpretation. The test_decoding does not decode
anything and there are no external "decoders". The decoding happens in
core, the decoding just provides C API for plugins to consume the
decoded info (ie, heap tuples and similar). The test_decoding *tests*
the decoding API and the external projects use the decoding API as well
but they don't really decode anything, their role is of filtering and
deciding the wire protocol mostly.

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] PG 10 release notes

2017-05-05 Thread Petr Jelinek
On 05/05/17 03:26, Andres Freund wrote:
> On 2017-05-04 18:23:38 -0700, Peter Geoghegan wrote:
>> On Thu, May 4, 2017 at 6:08 PM, Robert Haas <robertmh...@gmail.com> wrote:
>>>> E.g. to power amazon's data migration service (yes, that scares me).
>>>
>>> If you recall, I did predict prior to commit that test_decoding would
>>> get put into production use regardless of the name.
>>
>> I thought you were completely wrong when you said that. Lesson
>> learned, I suppose.
> 
> At least I was a bit more optimistic that we'd get a decent json plugin
> into care soon-ish.  While there was some initial work towards that, it
> unfortunately stalled at some point.
> 
> Euler, are you perchance interested in working towards that? ;)
> 

I am happy to review any work in this area BTW.

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] compiler warning with VS 2017

2017-05-05 Thread Petr Jelinek
On 05/05/17 06:50, Tom Lane wrote:
> Haribabu Kommi <kommi.harib...@gmail.com> writes:
>> I am getting a compiler warning when I build the latest HEAD PostgreSQL with
>> visual studio 2017.
>> The code at the line is,
>> tuple->values[i] = (char *) (Size)0xdeadbeef; /* make bad usage more obvious 
>> */
> 
> Yeah, you're not the first to complain about this.  To my mind that
> coding is not pretty, not cute, and not portable: there's not even
> a good reason to believe that dereferencing the pointer would result
> in a crash.  Perhaps the author can explain to us why this is better
> than just assigning NULL.
> 
> Actually, looking around a bit there, it's not even clear why
> we should be booby-trapping the value of an unchanged column in
> the first place.  So I'd say that not only is the code dubious
> but the comment is inadequate too.

Hmm, as far as I can recollect this is just leftover debugging code that
was intended to help ensure that we are checking the "changed"
everywhere we are supposed to (since I changed handling of these
structured quite a bit during development). Should be changed to NULL,
that's what we usually do in this type of situation.

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] snapbuild woes

2017-05-05 Thread Petr Jelinek
On 05/05/17 02:42, Andres Freund wrote:
> On 2017-05-04 17:00:04 -0700, Andres Freund wrote:
>> Attached is a prototype patch for that.
> 

I am not sure I understand the ABI comment for started_collection_at.
What's the ABI issue? The struct is private to snapbuild.c module. Or
you want to store it in the ondisk snapshot as well?

About better name for it what about something like oldest_full_xact?

Otherwise the logic seems to be right on first read, will ponder it a
bit more

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Logical replication - TRAP: FailedAssertion in pgstat.c

2017-05-05 Thread Petr Jelinek
On 03/05/17 13:23, Erik Rijkers wrote:
> On 2017-05-03 08:17, Petr Jelinek wrote:
>> On 02/05/17 20:43, Robert Haas wrote:
>>> On Thu, Apr 20, 2017 at 2:58 PM, Peter Eisentraut
> 
>>>> code path that calls CommitTransactionCommand() should have one, no?
>>>
>>> Is there anything left to be committed here?
>>>
>>
>> Afaics the fix was not committed. Peter wanted more comprehensive fix
>> which didn't happen. I think something like attached should do the job.
> 
> I'm running my pgbench-over-logical-replication test in chunk of 15
> minutes, wth different pgbench -c (num clients) and -s (scale) values.
> 
> With this patch (and nothing else)  on top of master (8f8b9be51fd7 to be
> precise):
> 
>> fix-statistics-reporting-in-logical-replication-work.patch
> 
> logical replication is still often failing (as expected, I suppose; it
> seems because of "inital snapshot too large") but indeed I do not see

Yes that's different thing that we've been discussing a bit in snapbuild
woes thread.

> the 'TRAP: FailedAssertion in pgstat.c' anymore.
> 
> (If there is any other configuration of patches worth testing please let
> me know)
> 

Thanks, so the patch works.

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] snapbuild woes

2017-05-05 Thread Petr Jelinek
On 05/05/17 02:00, Andres Freund wrote:
> Hi,
> 
> On 2017-05-02 08:55:53 +0200, Petr Jelinek wrote:
>> Aah, now I understand we talked about slightly different things, I
>> considered the running thing to be first step towards tracking aborted
>> txes everywhere.
>> I think
>> we'll have to revisit tracking of aborted transactions in PG11 then
>> though because of the 'snapshot too large' issue when exporting, at
>> least I don't see any other way to fix that.
> 
> FWIW, that seems unnecessary - we can just check for that using the
> clog.  Should be very simple to check for aborted xacts when exporting
> the snapshot (like 2 lines + comments).  That should address your
> concern, right?

Right, because there isn't practical difference between running and
aborted transaction for us so we don't mind if the abort has happened in
the future. Yeah the export code could do the check seems quite simple.

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] logical replication syntax (was DROP SUBSCRIPTION, query cancellations and slot handling)

2017-05-04 Thread Petr Jelinek
On 04/05/17 23:29, Tom Lane wrote:
> Robert Haas <robertmh...@gmail.com> writes:
>> On Wed, May 3, 2017 at 12:38 AM, Petr Jelinek
>> <petr.jeli...@2ndquadrant.com> wrote:
>>> Ok, Let me be clear, I actually happen to agree with your proposal. The
>>> reason I am moaning is that I always seem to find myself doing tons of
>>> mechanical work to rewrite some cosmetic aspect of some patch based on
>>> which committer is paying attention in a specific week. So while I am
>>> for doing exactly what you proposed, I'd like to see couple of +1s first
>>> (Peter?) since I don't want to rewrite it to something different again
>>> and it's also long past freeze.
> 
>> So, Tom Lane and Thom Brown and Josh Drake all seemed generally in
>> favor of cleaning this up.  Perhaps they could opine on this
>> particular proposal.
> 
> It seems like there's some remaining indecision between "make it look
> like the options in EXPLAIN, VACUUM, etc" and "make it look like the
> WITH options found in some other statements".  I do not have a strong
> opinion which one to do, but I'd definitely say that you should use WITH
> in the latter case but not in the former.  I think this mostly boils down
> to whether to use "=" or not; you've got "not" in the proposal, which
> means you are following the EXPLAIN precedent and should not use WITH.
> 

Okay, here is my initial attempt on changing this. I opted for WITH and
"=" as I like it slightly better (also the generic_options expect values
to be quoted which I don't like and then I would have to again invent
something like generic_options but not quite the same).

Most of the changes go to doc and tests, not that much code has changed
as I already used the definiton (which is the parser's name for these
WITH things). Except that I removed the NO shorthands and changed
publish_insert,etc to publish = 'insert,...'. I also changed the
NOREFRESH to SKIP REFRESH.

I didn't touch the DROP SUBSCRIPTION slot handling so far, that needs to
be separate patch as there is behavior change there, while this is
purely cosmetic and IMHO it's better to not mix those. (I plan to send
patch for that which changes the behavior heavily soonish as well)

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


Rework-the-options-for-logical-replication.patch
Description: binary/octet-stream

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] CTE inlining

2017-05-04 Thread Petr Jelinek
On 03/05/17 23:24, Merlin Moncure wrote:
> On Wed, May 3, 2017 at 12:33 PM, Alvaro Herrera
> <alvhe...@2ndquadrant.com> wrote:
>> David Fetter wrote:
>>
>>> When we add a "temporary" GUC, we're taking on a gigantic burden.
>>> Either we support it forever somehow, or we put it on a deprecation
>>> schedule immediately and expect to be answering questions about it for
>>> years after it's been removed.
>>>
>>> -1 for the GUC.
>>
>> Absolutely.
>>
>> So ISTM we have three choices:
>>
>> 1) we switch unmarked CTEs as inlineable by default in pg11.  What seems
>> likely to happen for a user that upgrades to pg11 is that 5 out of 10
>> CTE-using queries are going to become faster than with pg10, and they
>> are going to be happy; 4 out of five are going to see no difference, but
>> they didn't have to do anything about it; and the remaining query is
>> going to become slower, either indistinguishably so (in which case they
>> don't care and they remain happy because of the other improvements) or
>> notably so, in which case they can easily figure where to add the
>> MATERIALIZED option and regain the original performance.
> 
> +1 for option 1.  This change will be welcome for a large number of
> queries, but forced materialization is a real need and I use it often.
> This comes off as a very reasonable compromise in my opinion unless it
> requires major coding gymnastics to implement.
> 

+1 to this

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] snapbuild woes

2017-05-04 Thread Petr Jelinek
On 04/05/17 07:45, Noah Misch wrote:
> On Thu, Apr 27, 2017 at 09:42:58PM -0700, Andres Freund wrote:
>>
>>
>> On April 27, 2017 9:34:44 PM PDT, Noah Misch <n...@leadboat.com> wrote:
>>> On Fri, Apr 21, 2017 at 10:36:21PM -0700, Andres Freund wrote:
>>>> On 2017-04-17 21:16:57 -0700, Andres Freund wrote:
>>>>> I've since the previous update reviewed Petr's patch, which he
>>> since has
>>>>> updated over the weekend.  I'll do another round tomorrow, and will
>>> see
>>>>> how it looks.  I think we might need some more tests for this to be
>>>>> committable, so it might not become committable tomorrow.  I hope
>>> we'll
>>>>> have something in tree by end of this week, if not I'll send an
>>> update.
>>>>
>>>> I was less productive this week than I'd hoped, and creating a
>>> testsuite
>>>> was more work than I'd anticipated, so I'm slightly lagging behind. 
>>> I
>>>> hope to have a patchset tomorrow, aiming to commit something
>>>> Monday/Tuesday.
>>>
>>> This PostgreSQL 10 open item is past due for your status update. 
>>> Kindly send
>>> a status update within 24 hours, and include a date for your subsequent
>>> status
>>> update.  Refer to the policy on open item ownership:
>>> https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com
>>
>> I committed part of the series today, plan to continue doing so over the 
>> next few days.  Changes require careful review & testing, this is easy to 
>> get wrong...
> 
> This PostgreSQL 10 open item is past due for your status update.  Kindly send
> a status update within 24 hours, and include a date for your subsequent status
> update.
> 
> Also, this open item has been alive for three weeks, well above guideline.  I
> understand it's a tricky bug, but I'm worried this isn't on track to end.
> What is missing to make it end?

It's tricky 5 bugs, and they are quite sensitive on rare
timing/concurrency events.

First two are fixed, we can live with 5th being done later (as it's not
correctness fix, but very bad performance one).

We haven't figured way for fixing the 4th one that we all agree is good
solution (everything proposed so far still had bugs).

I am not quite sure what happened to the 3rd one.

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] logical replication syntax (was DROP SUBSCRIPTION, query cancellations and slot handling)

2017-05-03 Thread Petr Jelinek
On 02/05/17 16:14, Petr Jelinek wrote:
> On 02/05/17 15:31, Tom Lane wrote:
>> Petr Jelinek <petr.jeli...@2ndquadrant.com> writes:
>>> Let me expand, if we don't drop the slot by default when dropping
>>> subscription, we'll have a lot of users with dead slots laying around
>>> holding back WAL/catalog_xmin, that's really bad. If we do drop by
>>> default like now, we need option to not do that, neither RESTRICT, nor
>>> CASCADE fit that.
>>
>> I'm not sure which part of "you can't have an option in DROP" isn't
>> clear to you.  Whatever the default behavior is always has to work,
>> because that is what's going to happen during DROP OWNED BY, or
>> DROP DATABASE. 
> 
> You are saying it like there was some guarantee that those commands
> can't fail because of other objects. AFAIK for example drop database can
> trivially fail just because there is replication slot in it before PG10
> (IIRC Craig managed to fix that in 10 for unrelated reasons).
> 

Btw looks like I already forgot how this stuff behaves. Existence of
subscription currently also prevents DROP DATABASE (for same reason
active backends do).

DROP OWNED BY ignores SUBSCRIPTION too now, although I think it might
not be strictly necessary to stay that way.

But if we keep this behavior then the point about these two commands
cascading to subscriptions is largely moot.

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Time based lag tracking for logical replication

2017-05-03 Thread Petr Jelinek
On 03/05/17 08:28, Simon Riggs wrote:
> On 23 April 2017 at 01:10, Petr Jelinek <petr.jeli...@2ndquadrant.com> wrote:
>> Hi,
>>
>> The time based lag tracking commit [1] added interface for logging
>> progress of replication so that we can report lag as time interval
>> instead of just bytes. But the patch didn't contain patch for the
>> builtin logical replication.
>>
>> So I wrote something that implements this. I didn't like all that much
>> the API layering in terms of exporting the walsender's LagTrackerWrite()
>> for use by plugin directly. Normally output plugin does not have to care
>> if it's running under walsender or not, it uses abstracted write
>> interface for that which can be implemented in various ways (that's how
>> we implement SQL interface to logical decoding after all). So I decided
>> to add another function to the logical decoding write api called
>> update_progress and call that one from the output plugin. The walsender
>> then implements that new API to call the LagTrackerWrite() while the SQL
>> interface just does not implement it at all. This seems like cleaner way
>> of doing it.
>>
>> Thoughts?
> 
> Agree cleaner.
> 
> I don't see any pacing or comments about it, nor handling of
> intermediate messages while we process a large transaction.

Agreed, pacing is good idea because on busy server storing info for
every commit could get expensive.

Don't understand what you mean by "handling of intermediate messages
while we process a large transaction". Logical replication is
transaction based so far, it does not stream like physical replication
so it seems like there is limited usefulness in doing this outside of
commit no?

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Logical replication - TRAP: FailedAssertion in pgstat.c

2017-05-03 Thread Petr Jelinek
On 02/05/17 20:43, Robert Haas wrote:
> On Thu, Apr 20, 2017 at 2:58 PM, Peter Eisentraut
> <peter.eisentr...@2ndquadrant.com> wrote:
>> On 4/16/17 16:11, Petr Jelinek wrote:
>>> Yeah it is, it needs to be fenced to happen only after commit, which is
>>> not guaranteed at the point of code, we probably need to put the
>>> pgstat_report_stat() inside the if above after the
>>> CommitTransactionCommand() (that will make it report stats for changes
>>> apply did to pg_subscription_rel after next transaction though)
>>
>> I think to avoid the latter, we should add more pgstat_report_stat()
>> calls, such as in process_syncing_tables_for_apply().  Basically every
>> code path that calls CommitTransactionCommand() should have one, no?
> 
> Is there anything left to be committed here?
> 

Afaics the fix was not committed. Peter wanted more comprehensive fix
which didn't happen. I think something like attached should do the job.

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


fix-statistics-reporting-in-logical-replication-work.patch
Description: binary/octet-stream

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] logical replication syntax (was DROP SUBSCRIPTION, query cancellations and slot handling)

2017-05-02 Thread Petr Jelinek
On 02/05/17 18:25, Alvaro Herrera wrote:
> Petr Jelinek wrote:
>> On 02/05/17 18:00, Robert Haas wrote:
>>> On Tue, May 2, 2017 at 11:49 AM, Alvaro Herrera
>>> <alvhe...@2ndquadrant.com> wrote:
>>>> Petr Jelinek wrote:
>>>>> So the only way to fulfill the requirement you stated is to just not try
>>>>> to drop the slot, ever, on DROP SUBSCRIPTION. That makes the default
>>>>> behavior leave resources on upstream that will eventually cause that
>>>>> server to stop unless user notices before. I think we better invent
>>>>> something that limits how much inactive slots can hold back WAL and
>>>>> catalog_xmin in this release as well then.
>>>>
>>>> I don't understand why isn't the default behavior to unconditionally
>>>> drop the slot.  Why do we ever want the slot to be kept?
>>>
>>> What if the remote server doesn't exist any more?
>>
>> Or what if the slot is used by other subscription (because you restored
>> dump containing subscriptions to another server for example), or you
>> have server that does not have outside network access anymore, or many
>> other reasons.
> 
> So there are two different scenarios: one is that you expect the slot
> drop to fail for whatever reason; the other is that you want the slot to
> be kept because it's needed for something else.  Maybe those two should
> be considered separately.
> 
> 1) keep the slot because it's needed for something else.
>I see two options:
>a) change the something else so that it uses another slot with the
>   same location.  Do we have some sort of "clone slot" operation?

Nope.

>b) have an option to dissociate the slot from the subscription prior
>   to the drop.
> 

We do have ALTER SUBSCRIPTION bla WITH (SLOT NAME = 'something') so this
is definitely doable but ALTER SUBSCRIPTION bla WITH (SLOT NAME = '') is
not very nice syntax, maybe something like RESET SLOT NAME?

> 2) don't drop because we know it won't work.  I see two options:
>c) ignore a drop slot failure, i.e. don't cause a transaction abort.
>   An easy way to implement this is just add a PG_TRY block, but we
>   dislike adding those and not re-throwing the error.
>d) rethink drop slot completely; maybe instead of doing it
>   immediately, it should be a separate task, so we first close the
>   current transaction (which dropped the subscription) and then we open
>   a second one to drop the slot, so that if the drop slot fails, the
>   subscription does not come back to life.
> 

Yeah I was thinking about ignoring failures with WARNING as well, but
never seemed right because it would not solve the case 1), but I didn't
think of b) which might solve it.

I was also thinking on how to map the behavior to RESTRICT vs CASCADE,
wonder if RESTRICT should check for slot existence and refuse to drop if
the slot exists (question is then should it bail on connection failure
or ignore it?) and CASCADE should try to drop the slot and only warn on
failure then.

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] logical replication syntax (was DROP SUBSCRIPTION, query cancellations and slot handling)

2017-05-02 Thread Petr Jelinek
On 02/05/17 22:40, Robert Haas wrote:
> On Tue, May 2, 2017 at 3:02 PM, Petr Jelinek
> <petr.jeli...@2ndquadrant.com> wrote:
>> That sounds okay. I know PeterE didn't like the lower case and
>> underscore separated words for options in the original patch, so I'd
>> like to hear his opinion on this. I am not sure how much advantage is
>> there in removing the '=' in between the key and value. That's the main
>> difference between generic options and definitions (well and definitions
>> can have 2 words for key, but that's something I have added anyway), I
>> don't really understand why we have both and use one for some commends
>> and the other for others btw.
> 
> I don't care all *that* much about the equals sign, although I think
> it's marginally more elegant without it, and VACUUM, EXPLAIN, and COPY
> all do it that way.  But I think allowing two-word names is just a
> pile of parser nastiness that we'd be far better off without.  If you
> use the same syntax as VACUUM, EXPLAIN, and COPY, all options are a
> single identifier.

Ok, Let me be clear, I actually happen to agree with your proposal. The
reason I am moaning is that I always seem to find myself doing tons of
mechanical work to rewrite some cosmetic aspect of some patch based on
which committer is paying attention in a specific week. So while I am
for doing exactly what you proposed, I'd like to see couple of +1s first
(Peter?) since I don't want to rewrite it to something different again
and it's also long past freeze.

To repeat the proposal:
- change the WITH (...) clauses in subscription/publication commands to:
(create_slot true/false, connect true/false, slot_name 'something',
copy_data true/false, etc)

- change the NOREFRESH to NO REFRESH in ALTER SUBSCRIPTION name SET
PUBLICATION (btw I originally had SKIP REFRESH there but changed it to
NOREFRESH for consistency with the other NO* stuff, wonder if SKIP would
sound more english).

- change the (publish insert/nopublish insert/publish update/nopublish
update), etc options to (publish 'update,insert').

And one question, if we are not using the definitions (key = value)
should we keep the WITH keyword in the syntax, would it be confusing?

>  If it's got to be multiple words, they can be
> separated by underscores.  But with what you've got right now, it's
> sometimes one identifier even when it's two words, and sometimes two
> identifiers. 
> 

The main inconsistency is the synchronous_commit which is that way to
make it clear it's same as the GUC it changes, but looks like that was a
mistake.

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] logical replication syntax (was DROP SUBSCRIPTION, query cancellations and slot handling)

2017-05-02 Thread Petr Jelinek
On 02/05/17 19:42, Robert Haas wrote:
> On Tue, May 2, 2017 at 8:45 AM, Petr Jelinek
> <petr.jeli...@2ndquadrant.com> wrote:
>> I am happy to implement something different, it's quite trivial to
>> change. But I am not going to propose anything different as I can't
>> think of better syntax (if I could I would have done it). I don't like
>> the OFF or FALSE (ie DROP SLOT OFF) any more than what is there now and
>> it also seems to not map very well to action (as opposed to output
>> option as it is in EXPLAIN). It might not be very close to SQL way but
>> that's because SQL way would be do not do any of those default actions
>> unless they are actually asked for (ie NODROP SLOT would be default and
>> DROP SLOT would be the option) but that's IMHO less user friendly.
> 
> So the cases where this "NO" prefixing comes up are:
> 
> 1. CREATE SUBSCRIPTION
> 
> where option can
> be:
> 
> | ENABLED | DISABLED
> | CREATE SLOT | NOCREATE SLOT
> | SLOT NAME = slot_name
> | COPY DATA | NOCOPY DATA
> | SYNCHRONOUS_COMMIT =  class="PARAMETER">synchronous_commit
> | NOCONNECT
> 
> I think it would have been a lot better to use the extensible options
> syntax for this instead of inventing something new that's not even
> consistent with itself.

I am not sure what you mean by this, we always have to invent option
names if they are new options, even if we use generic options (which I
guess is what you mean by "extensible options syntax"). I used the
definitions instead of generic options, this means that the supported
syntax also includes COPY DATA = true/false, CREATE SLOT = true/false
etc, the NO* are just shorthands, it's quite simple to remove those.

> You've got SYNCHRONOUS_COMMIT with a hyphen
> and CREATE SLOT with no hyphen and NOCOPY DATA with no hyphen and a
> space left out. With the extensible options syntax, this would be
> (enabled true/false, create_slot true/false, slot_name whatever,
> synchronous_commit true/false, connect true/false). If we're going to
> keep the present monstrosity, we can I think still change NOCONNECT to
> NO CONNECT, but there's no fixing NOCOPY DATA in this syntax model.

See above.

> 
> 2. ALTER SUBSCRIPTION
> 
> ALTER SUBSCRIPTION name SET PUBLICATION publication_name [, ...] {
> REFRESH WITH ( puboption [, ... ] ) | NOREFRESH }
> 
> There is no obvious reason why this could not have been spelled NO
> REFRESH instead of adding a new keyword.
> 
> 3. DROP SUBSCRIPTION
> 
> DROP SUBSCRIPTION [ IF EXISTS ] name [ DROP SLOT | NODROP SLOT ]
> 
> This is where we started, and I have nothing to add to what I (and
> Tom) have already said.
> 
> 4. CREATE PUBLICATION
> 
> CREATE PUBLICATION name
> [ FOR TABLE [ ONLY ]  class="parameter">table_name [ * ] [, ...]
>   | FOR ALL TABLES ]
> [ WITH ( option [, ... ] ) ]
> 
> where option can
> be:
> 
>   PUBLISH INSERT | NOPUBLISH INSERT
> | PUBLISH UPDATE | NOPUBLISH UPDATE
> | PUBLISH DELETE | NOPUBLISH DELETE
> 
> Again, the extensible options syntax like we use for EXPLAIN would
> have been better here.  You could have said (publish_insert
> true/false, publish_update true/false, publish_delete true/false), for
> instance, or combined them into a single option like (publish
> 'insert,update') to omit deletes.
> 
> So it doesn't actually look hard to get rid of all of the NO prefixes.
> 

That sounds okay. I know PeterE didn't like the lower case and
underscore separated words for options in the original patch, so I'd
like to hear his opinion on this. I am not sure how much advantage is
there in removing the '=' in between the key and value. That's the main
difference between generic options and definitions (well and definitions
can have 2 words for key, but that's something I have added anyway), I
don't really understand why we have both and use one for some commends
and the other for others btw.


-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] logical replication syntax (was DROP SUBSCRIPTION, query cancellations and slot handling)

2017-05-02 Thread Petr Jelinek
On 02/05/17 18:00, Robert Haas wrote:
> On Tue, May 2, 2017 at 11:49 AM, Alvaro Herrera
> <alvhe...@2ndquadrant.com> wrote:
>> Petr Jelinek wrote:
>>> So the only way to fulfill the requirement you stated is to just not try
>>> to drop the slot, ever, on DROP SUBSCRIPTION. That makes the default
>>> behavior leave resources on upstream that will eventually cause that
>>> server to stop unless user notices before. I think we better invent
>>> something that limits how much inactive slots can hold back WAL and
>>> catalog_xmin in this release as well then.
>>
>> I don't understand why isn't the default behavior to unconditionally
>> drop the slot.  Why do we ever want the slot to be kept?
> 
> What if the remote server doesn't exist any more?
> 

Or what if the slot is used by other subscription (because you restored
dump containing subscriptions to another server for example), or you
have server that does not have outside network access anymore, or many
other reasons.

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Logical replication in the same cluster

2017-05-02 Thread Petr Jelinek
On 02/05/17 16:37, Andres Freund wrote:
> On 2017-05-02 09:17:27 +0200, Petr Jelinek wrote:
>> Yes because otherwise we risk leaving slot on the upstream if the
>> command fails downstream.
> 
> Shouldn't temporary slots be able to solve that concern?  Create it as
> temporary at the beginning, mark it as permanent at the end?
> 

So we need ALTER_REPLICATION_SLOT? :)

But that aside, based on the conversation nearby [1], we'll see if we
even want to create slots in CREATE SUBSCRIPTION.

[1]
https://www.postgresql.org/message-id/flat/CA%2BTgmoZmkbpAWRzVKDVcHnTBkYjJEFS8%3D09RL-G3zgdozCLFHQ%40mail.gmail.com#CA+TgmoZmkbpAWRzVKDVcHnTBkYjJEFS8=09rl-g3zgdozcl...@mail.gmail.com

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] logical replication syntax (was DROP SUBSCRIPTION, query cancellations and slot handling)

2017-05-02 Thread Petr Jelinek
On 02/05/17 15:31, Tom Lane wrote:
> Petr Jelinek <petr.jeli...@2ndquadrant.com> writes:
>> Let me expand, if we don't drop the slot by default when dropping
>> subscription, we'll have a lot of users with dead slots laying around
>> holding back WAL/catalog_xmin, that's really bad. If we do drop by
>> default like now, we need option to not do that, neither RESTRICT, nor
>> CASCADE fit that.
> 
> I'm not sure which part of "you can't have an option in DROP" isn't
> clear to you.  Whatever the default behavior is always has to work,
> because that is what's going to happen during DROP OWNED BY, or
> DROP DATABASE. 

You are saying it like there was some guarantee that those commands
can't fail because of other objects. AFAIK for example drop database can
trivially fail just because there is replication slot in it before PG10
(IIRC Craig managed to fix that in 10 for unrelated reasons).


> If you want more than one behavior during DROP,
> you need to drive that off something represented as a persistent
> property of the object, not off an option in the DROP command.
> 

I don't see how changing behavior as object property will change
anything in terms of not failing to cascade. If you use CREATE or ALTER
to say that subscription must drop slot and that fails then the cascade
will still fail so then you need to run ALTER again to change that
property. I fail to see how that's easier than running the DROP directly.

So the only way to fulfill the requirement you stated is to just not try
to drop the slot, ever, on DROP SUBSCRIPTION. That makes the default
behavior leave resources on upstream that will eventually cause that
server to stop unless user notices before. I think we better invent
something that limits how much inactive slots can hold back WAL and
catalog_xmin in this release as well then.

We should also not create the slot (at least by default) on CREATE
SUBSCRIPTION to have some symmetry.

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] logical replication syntax (was DROP SUBSCRIPTION, query cancellations and slot handling)

2017-05-02 Thread Petr Jelinek
On 02/05/17 15:14, Petr Jelinek wrote:
> On 02/05/17 15:10, Tom Lane wrote:
>> Robert Haas <robertmh...@gmail.com> writes:
>>>> On Thu, Apr 20, 2017 at 7:46 AM, Petr Jelinek
>>>> <petr.jeli...@2ndquadrant.com> wrote:
>>>>> DROP SUBSCRIPTION mysub NODROP SLOT;
>>
>>>> I'm pretty uninspired by this choice of syntax.
>>
>> Actually, this command has got much worse problems than whether you like
>> the spelling of its option: it shouldn't have an option in the first
>> place.  I put it to you as an inviolable rule that no object DROP command
>> should ever have any options other than RESTRICT/CASCADE and IF EXISTS.
>> If it does, then you don't know what to do when the object is recursed
>> to by a cascaded drop.
>>
>> It's possible that we could allow exceptions to this rule for object types
>> that can never have any dependencies.  But a subscription doesn't qualify
>> --- it's got an owner, so DROP OWNED BY already poses this problem.  Looks
>> to me like it's got a dependency on a database, too.  (BTW, if
>> subscriptions are per-database, why is pg_subscription a shared catalog?
>> There were some pretty schizophrenic decisions here.)
> 
> Because otherwise we would need launcher process per database, not pretty.
> 
>>
>> So ISTM we need to get rid of the above-depicted syntax.  We could instead
>> have what-to-do-with-the-slot as a property of the subscription,
>> established at CREATE SUBSCRIPTION and possibly changed later by ALTER
>> SUBSCRIPTION.  Not quite sure what to call the option, maybe something
>> based on the concept of the subscription "owning" the slot.
>>
> 
> So what do you do if the upstream does not exist anymore when you are
> dropping subscription?
> 

Let me expand, if we don't drop the slot by default when dropping
subscription, we'll have a lot of users with dead slots laying around
holding back WAL/catalog_xmin, that's really bad. If we do drop by
default like now, we need option to not do that, neither RESTRICT, nor
CASCADE fit that.

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] logical replication syntax (was DROP SUBSCRIPTION, query cancellations and slot handling)

2017-05-02 Thread Petr Jelinek
On 02/05/17 15:10, Tom Lane wrote:
> Robert Haas <robertmh...@gmail.com> writes:
>>> On Thu, Apr 20, 2017 at 7:46 AM, Petr Jelinek
>>> <petr.jeli...@2ndquadrant.com> wrote:
>>>> DROP SUBSCRIPTION mysub NODROP SLOT;
> 
>>> I'm pretty uninspired by this choice of syntax.
> 
> Actually, this command has got much worse problems than whether you like
> the spelling of its option: it shouldn't have an option in the first
> place.  I put it to you as an inviolable rule that no object DROP command
> should ever have any options other than RESTRICT/CASCADE and IF EXISTS.
> If it does, then you don't know what to do when the object is recursed
> to by a cascaded drop.
> 
> It's possible that we could allow exceptions to this rule for object types
> that can never have any dependencies.  But a subscription doesn't qualify
> --- it's got an owner, so DROP OWNED BY already poses this problem.  Looks
> to me like it's got a dependency on a database, too.  (BTW, if
> subscriptions are per-database, why is pg_subscription a shared catalog?
> There were some pretty schizophrenic decisions here.)

Because otherwise we would need launcher process per database, not pretty.

> 
> So ISTM we need to get rid of the above-depicted syntax.  We could instead
> have what-to-do-with-the-slot as a property of the subscription,
> established at CREATE SUBSCRIPTION and possibly changed later by ALTER
> SUBSCRIPTION.  Not quite sure what to call the option, maybe something
> based on the concept of the subscription "owning" the slot.
> 

So what do you do if the upstream does not exist anymore when you are
dropping subscription?

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] logical replication syntax (was DROP SUBSCRIPTION, query cancellations and slot handling)

2017-05-02 Thread Petr Jelinek
On 02/05/17 14:13, Tom Lane wrote:
> Robert Haas <robertmh...@gmail.com> writes:
>> On Thu, Apr 20, 2017 at 7:46 AM, Petr Jelinek
>> <petr.jeli...@2ndquadrant.com> wrote:
>>> DROP SUBSCRIPTION mysub NODROP SLOT;
> 
>> I'm pretty uninspired by this choice of syntax.  Logical replication
>> seems to have added a whole bunch of syntax that involves prefixing
>> words with "no".  In various places, there's NODROP, NOREFRESH, NOCOPY
>> DATA, NOCONNECT, and NOPUBLISH.  But "NO" is not an English prefix,
>> and there's no precedent of which I'm aware for such SQL syntax.  In
>> most places, we've chosen to name the option and then the user set it
>> to "on" or "off".  So for example you type EXPLAIN (ANALYZE, TIMING
>> OFF) or EXPLAIN (ANALYZE, TIMING FALSE), not EXPLAIN (ANALYZE,
>> NOTIMING).  I think most of the logical replication stuff could have
>> been done this way pretty easily, but for some reason it picked a
>> completely different approach.
> 
> I tend to agree with this criticism, but it's not quite true that we
> don't do this anywhere else --- see CREATE USER for one example, where
> we have monstrosities like NOBYPASSRLS.  Still, at least those are single
> words without arguments.  "NODROP SLOT" is pretty ugly, not least
> because if you want to claim CREATE USER as syntax precedent, you ought
> to spell it NODROPSLOT.
> 
> Having said that, I doubt that anyone would argue that CREATE USER is
> anything but legacy syntax, or that our more recent syntax designs aren't
> better models to follow.
> 
> It's not quite too late to revisit the syntax of the log rep commands
> ... shall we add this as an open item?

I am happy to implement something different, it's quite trivial to
change. But I am not going to propose anything different as I can't
think of better syntax (if I could I would have done it). I don't like
the OFF or FALSE (ie DROP SLOT OFF) any more than what is there now and
it also seems to not map very well to action (as opposed to output
option as it is in EXPLAIN). It might not be very close to SQL way but
that's because SQL way would be do not do any of those default actions
unless they are actually asked for (ie NODROP SLOT would be default and
DROP SLOT would be the option) but that's IMHO less user friendly.

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Logical replication in the same cluster

2017-05-02 Thread Petr Jelinek
On 02/05/17 04:14, Peter Eisentraut wrote:
> On 4/30/17 20:31, Andres Freund wrote:
>> On 2017-04-26 23:41:51 +0200, Petr Jelinek wrote:
>>> Yes that's result of how logical replication slots work, the transaction
>>> that needs to finish is your transaction. It can be worked around by
>>> creating the slot manually via the SQL interface for example and create
>>> the subscription using WITH (NOCREATE SLOT, SLOT NAME = 'your slot') .
>>
>> Is there any chance the creation of the slot could be moved ahead, to
>> before an xid has been assigned?
> 
> The trend has rather been to do most of the stuff before creating the
> slot, so that all the error checking is done.  See for example
> dcb39c37c1d3b90115e1501af8efb7af59c341c3.
> 

Yes because otherwise we risk leaving slot on the upstream if the
command fails downstream. We could move the slot creation outside of tx
as I proposed, then we risk leaving subscription downstream without slot
upstream though but I think that's less of a issues from user
friendliness and mainly safety (slot reserves wal and global catalog
xmin while subscription does not reserve anything except the name of the
subscription).

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] logical replication and PANIC during shutdown checkpoint in publisher

2017-05-02 Thread Petr Jelinek
On 02/05/17 05:35, Michael Paquier wrote:
> On Tue, May 2, 2017 at 7:07 AM, Peter Eisentraut
> <peter.eisentr...@2ndquadrant.com> wrote:
>> On 4/25/17 21:47, Michael Paquier wrote:
>>> Attached is an updated patch to reflect that.
>>
>> I edited this a bit, here is a new version.
> 
> Thanks, looks fine for me.
> 
>> A variant approach would be to prohibit *all* new commands after
>> entering the "stopping" state, just let running commands run.  That way
>> we don't have to pick which individual commands are at risk.  I'm not
>> sure that we have covered everything here.
> 
> It seems to me that everything is covered. We are taking about
> creation and dropping of slots here, where standby snapshots can be
> created and SQL queries can be run when doing a tablesync meaning that
> FPWs could be taken in the context of the WAL sender. Blocking all
> commands would be surely safer I agree, but I see no reason to block
> things more than necessary.
> 

I don't think the code covers all because a) the SQL queries are not
covered at all that I can see and b) logical decoding can theoretically
do HOT pruning (even if the chance is really small) so it's not safe to
start logical replication either.

I wonder if this whole prevent thing should just be called
unconditionally on walsender that's connected to database
(am_db_walsender), because in the WAL logging will only happen there -
CREATE_REPLICATION_SLOT PHYSICAL will not WAL log and
CREATE_REPLICATION_SLOT LOGICAL can't be run without being connected to
db, neither can logical decoding and SQL queries, so that coves all.

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] snapbuild woes

2017-05-02 Thread Petr Jelinek
On 01/05/17 21:14, Andres Freund wrote:
> On 2017-05-01 11:09:44 +0200, Petr Jelinek wrote:
>> On 01/05/17 10:03, Andres Freund wrote:
>>> On 2017-05-01 03:54:49 +0200, Petr Jelinek wrote:
> 
>>>> But, I still think we need to restart the tracking after new
>>>> xl_running_xacts. Reason for that is afaics any of the catalog snapshots
>>>> that we assigned to transactions at the end of SnapBuildCommitTxn might
>>>> be corrupted otherwise as they were built before we knew one of the
>>>> supposedly running txes was actually already committed and that
>>>> transaction might have done catalog changes.
>>>
>>> I'm afraid you're right.  But I think this is even more complicated: The
>>> argument in your version that this can only happen once, seems to also
>>> be holey: Just imagine a pg_usleep(3000 * 100) right before
>>> ProcArrayEndTransaction() and enjoy the picture.
> 
>> Well yes, transaction can in theory have written commit/abort xlog
>> record and stayed in proc for more than single xl_running_xacts write.
>> But then the condition which we test that the new xl_running_xacts has
>> bigger xmin than the previously tracked one's xmax would not be
>> satisfied and we would not enter the relevant code path yet. So I think
>> we should not be able to get any xids we didn't see. But we have to
>> restart tracking from beginning (after first checking if we didn't
>> already see anything that the xl_running_xacts considers as running),
>> that's what my code did.
> 
> But to get that correct, we'd have to not only track ->committed, but
> also somehow maintain ->aborted, and not just for the transactions in
> the original set of running transactions.  That'd be fairly complicated
> and large.  The reason I was trying - and it's definitely not correct as
> I had proposed - to use the original running_xacts record is that that
> only required tracking as many transaction statuses as in the first
> xl_running_xacts.  Am I missing something?

Aah, now I understand we talked about slightly different things, I
considered the running thing to be first step towards tracking aborted
txes everywhere. I am not sure if it's complicated, it would be exactly
the same as committed tracking, except we'd do it only before we reach
SNAPBUILD_CONSISTENT. It would be definitely larger patch I agree, but I
can give it at try.

If you think that adding the SNAPBUILD_BUILD_INITIAL_SNAPSHOT would be
less invasive/smaller patch I am okay with doing that for PG10. I think
we'll have to revisit tracking of aborted transactions in PG11 then
though because of the 'snapshot too large' issue when exporting, at
least I don't see any other way to fix that.

> 
> The probabilistic tests catch the issues here fairly quickly, btw, if
> you run with synchronous_commit=on, while pgbench is running, because
> the WAL flushes make this more likely.  Runs this query:
> 
> SELECT account_count, teller_count, account_sum - teller_sum s
> FROM
> (
> SELECT count(*) account_count, SUM(abalance) account_sum
> FROM pgbench_accounts
> ) a,
> (
> SELECT count(*) teller_count, SUM(tbalance) teller_sum
> FROM pgbench_tellers
> ) t
> 
> which, for my scale, should always return:
> ┌─┬─┬───┐
> │a│  t  │ s │
> ├─┼─┼───┤
> │ 200 │ 200 │ 0 │
> └─┴─┴───┘
> but with my POC patch occasionally returns things like:
> ┌─┬─┬───┐
> │a│  t  │   s   │
> ├─┼─┼───┤
> │ 200 │ 212 │ 37358 │
> └─┴─┴───┘
> 
> which obviously shouldn't be the case.
> 

Very nice (the test, not the failures ;)) !

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] snapbuild woes

2017-05-01 Thread Petr Jelinek
On 01/05/17 10:03, Andres Freund wrote:
> On 2017-05-01 03:54:49 +0200, Petr Jelinek wrote:
>> I agree with adding running, I think that's good thing even for the per
>> transaction tracking and snapshot exports - we could use the newly added
>> field to get rid of the issue we have with 'snapshot too large' when
>> there were many aborted transactions while we waited for running ones to
>> finish.
> 
> I'm not sure of that - what I was proposing would only track this for
> the ->running substructure.  How'd that help?
> 

Well not as is, but it's a building block for it.

> 
>> But, I still think we need to restart the tracking after new
>> xl_running_xacts. Reason for that is afaics any of the catalog snapshots
>> that we assigned to transactions at the end of SnapBuildCommitTxn might
>> be corrupted otherwise as they were built before we knew one of the
>> supposedly running txes was actually already committed and that
>> transaction might have done catalog changes.
> 
> I'm afraid you're right.  But I think this is even more complicated: The
> argument in your version that this can only happen once, seems to also
> be holey: Just imagine a pg_usleep(3000 * 100) right before
> ProcArrayEndTransaction() and enjoy the picture.
>

Well yes, transaction can in theory have written commit/abort xlog
record and stayed in proc for more than single xl_running_xacts write.
But then the condition which we test that the new xl_running_xacts has
bigger xmin than the previously tracked one's xmax would not be
satisfied and we would not enter the relevant code path yet. So I think
we should not be able to get any xids we didn't see. But we have to
restart tracking from beginning (after first checking if we didn't
already see anything that the xl_running_xacts considers as running),
that's what my code did.

> Wonder if we should just (re-)add a stage between SNAPBUILD_START and
> SNAPBUILD_FULL_SNAPSHOT.  Enter SNAPBUILD_BUILD_INITIAL_SNAPSHOT at the
> first xl_running_xacts, wait for all transactions to end with my
> approach, while populating SnapBuild->committed, only then start
> collecting changes for transactions (i.e. return true from
> SnapBuildProcessChange()), return true once all xacts have finished
> again.  That'd presumably be a bit easier to understand, more robust -
> and slower.
> 

That would also work, but per above, I don't understand why it's needed.

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] snapbuild woes

2017-04-30 Thread Petr Jelinek
On 01/05/17 04:29, Craig Ringer wrote:
> On 1 May 2017 at 09:54, Petr Jelinek <petr.jeli...@2ndquadrant.com> wrote:
> 
>> But, I still think we need to restart the tracking after new
>> xl_running_xacts. Reason for that is afaics any of the catalog snapshots
>> that we assigned to transactions at the end of SnapBuildCommitTxn might
>> be corrupted otherwise as they were built before we knew one of the
>> supposedly running txes was actually already committed and that
>> transaction might have done catalog changes.
> 
> Due to the race where LogStandbySnapshot() collects running-xacts info
> while a concurrent xact commits, such that the xl_xact_commit appears
> before the xl_running_xacts, but the xl_running_xacts still has the
> commited xact listed as running, right? Because we update PGXACT only
> after we write the commit to WAL, so there's a window where an xact is
> committed in WAL but not shown as committed in shmem.
> 

Yes that's what the patch at hand tries to fix, but Andres approached it
from too simplistic standpoint as we don't only care about the exported
snapshot but whatever catalog snapshots we made for the transactions we
already track, unless I am missing something.

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] snapbuild woes

2017-04-30 Thread Petr Jelinek
On 01/05/17 03:35, Andres Freund wrote:
> On 2017-04-30 17:59:21 -0700, Andres Freund wrote:
>> ISTM, we need a xip_status array in SnapBuild->running.  Then, whenever
>> a xl_running_xacts is encountered while builder->running.xcnt > 0, loop
>> for i in 0 .. xcnt_space, and end SnapBuildEndTxn() if xip_status[i] but
>> the xid is not xl_running_xacts?  Does that make sense?
> 
> A hasty implementation, untested besides check-world, of that approach
> is attached.  I'm going out for dinner now, will subject it to mean
> things afterwards.
> 
> Needs more testing, comment and commit message policing.  But I think
> the idea is sound?
> 

I agree with adding running, I think that's good thing even for the per
transaction tracking and snapshot exports - we could use the newly added
field to get rid of the issue we have with 'snapshot too large' when
there were many aborted transactions while we waited for running ones to
finish. Because so far only tracked committed transactions, any aborted
one would show as running inside the exported snapshot so it can grow
over maximum number of backends and can't be exported anymore. So +1 for
that part.

But, I still think we need to restart the tracking after new
xl_running_xacts. Reason for that is afaics any of the catalog snapshots
that we assigned to transactions at the end of SnapBuildCommitTxn might
be corrupted otherwise as they were built before we knew one of the
supposedly running txes was actually already committed and that
transaction might have done catalog changes.

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Time based lag tracking for logical replication

2017-04-29 Thread Petr Jelinek
On 23/04/17 01:10, Petr Jelinek wrote:
> Hi,
> 
> The time based lag tracking commit [1] added interface for logging
> progress of replication so that we can report lag as time interval
> instead of just bytes. But the patch didn't contain patch for the
> builtin logical replication.
> 
> So I wrote something that implements this. I didn't like all that much
> the API layering in terms of exporting the walsender's LagTrackerWrite()
> for use by plugin directly. Normally output plugin does not have to care
> if it's running under walsender or not, it uses abstracted write
> interface for that which can be implemented in various ways (that's how
> we implement SQL interface to logical decoding after all). So I decided
> to add another function to the logical decoding write api called
> update_progress and call that one from the output plugin. The walsender
> then implements that new API to call the LagTrackerWrite() while the SQL
> interface just does not implement it at all. This seems like cleaner way
> of doing it.
> 

The original no longer applies after Andres committed some of my fixes
for snapshot builder so here is rebased version.

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services



Add-support-for-time-based-lag-tracking-to-logical-170429.patch
Description: binary/octet-stream

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Interval for launching the table sync worker

2017-04-27 Thread Petr Jelinek
On 27/04/17 21:00, Peter Eisentraut wrote:
> On 4/27/17 06:47, Petr Jelinek wrote:
>> One thing I am missing in your patch however is cleanup of entries for
>> relations that finished sync. I wonder if it would be enough to just
>> destroy the hash when we get to empty list.
> 
> I had omitted that because the amount of memory "leaked" is not much,
> but I guess it wouldn't hurt to clean it up.
> 
> How about the attached?
> 

Yes that's more or less what I meant. All good.

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Interval for launching the table sync worker

2017-04-27 Thread Petr Jelinek
On 25/04/17 19:54, Peter Eisentraut wrote:
> I feel it's getting a bit late for reworkings of this extent, also
> considering the marginal nature of the problem we are trying to fix.  My
> patch from April 18 is very localized and gets the job done.
> 
> I think this is still a good direction to investigate, but if we have to
> extend the hash table API to get it done, this might not be the best time.
> 

Yeah the hash API change needed is a bummer at this stage.

One thing I am missing in your patch however is cleanup of entries for
relations that finished sync. I wonder if it would be enough to just
destroy the hash when we get to empty list.

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] some review comments on logical rep code

2017-04-27 Thread Petr Jelinek
On 26/04/17 18:36, Fujii Masao wrote:
> On Thu, Apr 27, 2017 at 1:28 AM, Fujii Masao <masao.fu...@gmail.com> wrote:
>> On Wed, Apr 26, 2017 at 3:47 PM, Kyotaro HORIGUCHI
>> <horiguchi.kyot...@lab.ntt.co.jp> wrote:
>>> At Wed, 26 Apr 2017 14:31:12 +0900, Masahiko Sawada <sawada.m...@gmail.com> 
>>> wrote in 
>>> <CAD21AoDMy8a6UwMrRh8pigQbDC+JAOQ4m_tXT41VRP4SYp23=w...@mail.gmail.com>
>>>> On Wed, Apr 26, 2017 at 12:35 PM, Petr Jelinek
>>>> <petr.jeli...@2ndquadrant.com> wrote:
>>>>> On 26/04/17 01:01, Fujii Masao wrote:
>>>>>>>> However this is overkill for small gain and false wakeup of the
>>>>>>>> launcher is not so harmful (probably we can live with that), so
>>>>>>>> we do nothing here for this issue.
>>>>>>>
>>>>>>> I agree this as a whole. But I think that the main issue here is
>>>>>>> not false wakeups, but 'possible delay of launching new workers
>>>>>>> by 3 minutes at most' (this is centainly a kind of false wakeups,
>>>>>>> though). We can live with this failure when using two-paase
>>>>>>> commmit, but I think it shouldn't happen silently.
>>>>>>>
>>>>>>>
>>>>>>> How about providing AtPrepare_ApplyLauncher(void) like the
>>>>>>> follows and calling it in PrepareTransaction?
>>>>>>
>>>>>> Or we should apply the attached patch and handle the 2PC case properly?
>>>>>> I was thinking that it's overkill more than necessary, but that seems 
>>>>>> not true
>>>>>> as far as I implement that.
>>>>>>
>>>>> Looks like it does not even increase size of the 2pc file, +1 for this.
>>>>
>>>> In my honest opinion, I didn't have a big will that we should handle
>>>> even two-phase commit case, because this case is very rare (I could
>>>> not image such case) and doesn't mean to lead a harmful result such as
>>>> crash of server and returning inconsistent result. it just delays the
>>>> launching worker for at most 3 minutes. We also can deal with this for
>>>> example by making maximum nap time of apply launcher user-configurable
>>>> and document it.
>>>> But if we can deal with it by minimum changes like attached your patch I 
>>>> agree.
>>>
>>> This change looks reasonable to me, +1 from me to this.
>>>
>>> The patch reads on_commit_launcher_wakeup directly then updates
>>> it via ApplyALuncherWakeupAtCommit() but it's too much to add a
>>> function for the sake of this.
>>
>> OK, so what about the attached patch? I replaced all the calls to
>> ApplyLauncherWakeupAtCommit() with the code "on_commit_launcher_wakeup = 
>> true".
> 
> BTW, while I was reading the code to implement the patch that
> I posted upthread, I found that the following condition doesn't
> work as expected. This is because "last_start_time" is always 0.
> 
> /* Limit the start retry to once a wal_retrieve_retry_interval */
> if (TimestampDifferenceExceeds(last_start_time, now,
>   wal_retrieve_retry_interval))
> 
> "last_start_time" is set to "now" when the launcher starts up new
> worker. But "last_start_time" is defined and always initialized with 0
> at the beginning of the main loop in ApplyLauncherMain(), so
> the above condition becomes always true. This is obviously a bug.
> Attached patch fixes this issue.
> 

Yes, please go ahead and commit this.

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Logical replication in the same cluster

2017-04-27 Thread Petr Jelinek
On 27/04/17 04:50, Tom Lane wrote:
> Peter Eisentraut <peter.eisentr...@2ndquadrant.com> writes:
>>>> If that's a predictable deadlock, I think a minimum expectation is that
>>>> the system should notice it and throw an error, not just hang.
> 
>> We had some discussions early on about detecting connections to the same
>> server, but it's not entirely clear how to do that and it didn't seem
>> worth it at the time.
> 
> I wonder whether we actually need to detect connections to the same
> server per se.  I'm thinking about the one end taking some special
> heavyweight lock, and the other end taking the same lock, which would
> generally be free as long as the two ends aren't on the same server.
> Cascading replication might be a problem though ...
> 

Well cascading might not be problem. I mean this issue exists only
during the slot creation which is one time operation. Which is why the
workaround solves it. But I don't see what we could lock that's common
between publisher and subscriber unless we invent some database object
specifically for this purpose.

My idea in the original thread was to put the info about xid and sysid
somewhere in shmem when creating subscription and checking that on the
other side if the sysid is same as local one and the xid is active. It
would serialize the subscription creation but I don't see that as big
issue, it's not like it's common to create thousands of them in parallel
nor it is something where we care about shaving microseconds of runtime.

Back when writing the original patch set, I was also playing with the
idea of having CREATE SUBSCRIPTION do multiple committed steps in
similar fashion to CREATE INDEX CONCURRENTLY but that leaves mess behind
on failure which also wasn't very popular outcome. I wonder how bad it
would be if we created all the stuff for subscription but in disabled
form, then committed, then created slot outside of tx (slot creation is
not transactional anyway) and then switched the subscription to enabled
(if needed) in next tx. It would still leave subscription behind on
failure but a) user would see the failure, b) the subscription would be
inactive so no active harm from it. We also already prevent running
CREATE SUBSCRIPTION inside transaction block when automatic slot
creation is chosen so there is no difference from that perspective.

Just for info, in pglogical, we solve this by having the worker create
slot, not the user command, so then it just works. The reason why I
didn't do this in core is that from practice it does not seem to be very
user friendly in case there are errors (not enough slots free,
connecting not only to same server but same db, etc) because they will
only see the errors in logs after the fact (and often they don't look).
I am already unhappy about the fact that we have no facility for
bgworker to save the last error before dying into place that is
accessible via SQL and I'd rather not hide even more errors in the log.

Note that the workaround for all of this is not all that complex, you do
same thing (create slot manually) you'd do for physical replication with
slots.

Thoughts?

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Logical replication in the same cluster

2017-04-26 Thread Petr Jelinek
On 26/04/17 18:59, Bruce Momjian wrote:
> I tried setting up logical replication on the same server between two
> different databases, and got, from database test:
> 
>   test=> CREATE TABLE test (x INT PRIMARY KEY);
>   CREATE TABLE
>   test=>
>   test=> INSERT INTO test VALUES (1);
>   INSERT 0 1
>   test=> CREATE PUBLICATION mypub FOR TABLE test;
>   CREATE PUBLICATION
> 
> then from database test2:
> 
>   test2=> CREATE TABLE test (x INT PRIMARY KEY);
>   CREATE TABLE
>   test2=> CREATE SUBSCRIPTION mysub CONNECTION 'dbname=test port=5432'
>   PUBLICATION mypub;
>   NOTICE:  synchronized table states
> 
> and it just hangs.  My server logs say:
> 
>   2017-04-26 12:50:53.694 EDT [29363] LOG:  logical decoding found initial
>   starting point at 0/15FF3E0
>   2017-04-26 12:50:53.694 EDT [29363] DETAIL:  1 transaction needs to
>   finish.
> 
> Is this expected?  I can get it working from two different clusters.
> 

Yes that's result of how logical replication slots work, the transaction
that needs to finish is your transaction. It can be worked around by
creating the slot manually via the SQL interface for example and create
the subscription using WITH (NOCREATE SLOT, SLOT NAME = 'your slot') .

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] some review comments on logical rep code

2017-04-25 Thread Petr Jelinek
On 26/04/17 01:01, Fujii Masao wrote:
> On Mon, Apr 24, 2017 at 7:57 PM, Kyotaro HORIGUCHI
> <horiguchi.kyot...@lab.ntt.co.jp> wrote:
>> Hello,
>>
>> At Mon, 24 Apr 2017 11:18:32 +0900, Masahiko Sawada <sawada.m...@gmail.com> 
>> wrote in <cad21aobu8mzdgx-stj3u+qkaej5rpnouotw4kfexc4xddnf...@mail.gmail.com>
>>>>> BEGIN;
>>>>> ALTER SUBSCRIPTION hoge_sub ENABLE;
>>>>> PREPARE TRANSACTION 'g';
>>>>> BEGIN;
>>>>> SELECT 1;
>>>>> COMMIT;  -- wake up the launcher at this point.
>>>>> COMMIT PREPARED 'g';
>>>>>
>>>>> Even if we wake up the launcher even when a ROLLBACK PREPARED, it's
>>>>> not a big deal to the launcher process actually. Compared with the
>>>>> complexly of changing the logic so that on_commit_launcher_wakeup
>>>>> corresponds to prepared transaction, we might want to accept this
>>>>> behavior.
>>>>
>>>> on_commit_launcher_wakeup needs to be recoreded in 2PC state
>>>> file to handle this issue properly. But I agree with you, that would
>>>> be overkill for small gain. So I'm thinking to add the following
>>>> comment into your patch and push it. Thought?
>>>>
>>>> -
>>>> Note that on_commit_launcher_wakeup flag doesn't work as expected in 2PC 
>>>> case.
>>>> For example, COMMIT PREPARED on the transaction enabling the flag
>>>> doesn't wake up
>>>> the logical replication launcher if ROLLBACK on another transaction
>>>> runs before it.
>>>> To handle this case properly, the flag needs to be recorded in 2PC
>>>> state file so that
>>>> COMMIT PREPARED and ROLLBACK PREPARED can determine whether to wake up
>>>> the launcher. However this is overkill for small gain and false wakeup
>>>> of the launcher
>>>> is not so harmful (probably we can live with that), so we do nothing
>>>> here for this issue.
>>>> -
>>>>
>>>
>>> Agreed.
>>>
>>> I added this comment to the patch and attached it.
>>
>> The following "However" may need a follwoing comma.
>>
>>> However this is overkill for small gain and false wakeup of the
>>> launcher is not so harmful (probably we can live with that), so
>>> we do nothing here for this issue.
>>
>> I agree this as a whole. But I think that the main issue here is
>> not false wakeups, but 'possible delay of launching new workers
>> by 3 minutes at most' (this is centainly a kind of false wakeups,
>> though). We can live with this failure when using two-paase
>> commmit, but I think it shouldn't happen silently.
>>
>>
>> How about providing AtPrepare_ApplyLauncher(void) like the
>> follows and calling it in PrepareTransaction?
> 
> Or we should apply the attached patch and handle the 2PC case properly?
> I was thinking that it's overkill more than necessary, but that seems not true
> as far as I implement that.
> 

Looks like it does not even increase size of the 2pc file, +1 for this.

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Separation walsender & normal backends

2017-04-25 Thread Petr Jelinek
On 25/04/17 17:13, Fujii Masao wrote:
> On Tue, Apr 25, 2017 at 11:34 PM, Tom Lane <t...@sss.pgh.pa.us> wrote:
>> Andres Freund <and...@anarazel.de> writes:
>>> I've for a while suspected that the separation & duplication of
>>> infrastructure between walsenders and normal backends isn't nice.
>>
>> I think we should consider a more radical solution: trying to put
>> general SQL query capability into the replication protocol was a
>> bad idea and we should revert it while we still can.  The uglinesses
>> you mention aren't merely implementation issues, they're an indication
>> that that concept is broken in itself.
> 
> I think that it's worth considering this option in order to "stabilize"
> logical replication stuff before the release. The table sync patch
> (which allows walsender to run normal queries) introduced such
> uglinesses and increased the complexity in logical rep code.
> OTOH, I believe that logical replication is still useful even without
> initial table sync feature. So reverting the table sync patch seems
> possible idea.
> 

I don't think that's good idea, the usefulness if much lower without the
initial copy. The original patch for this added new commands to
replication protocol, adding generic SQL interface was result of request
in the reviews.

I personally don't mind moving back my original idea of special commands
if that was the consensus, but previous consensus was to do SQL instead.

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] tablesync patch broke the assumption that logical rep depends on?

2017-04-25 Thread Petr Jelinek
On 22/04/17 22:09, Petr Jelinek wrote:
> On 21/04/17 16:31, Petr Jelinek wrote:
>> On 21/04/17 16:23, Peter Eisentraut wrote:
>>> On 4/21/17 10:11, Petr Jelinek wrote:
>>>> On 21/04/17 16:09, Peter Eisentraut wrote:
>>>>> On 4/20/17 14:29, Petr Jelinek wrote:
>>>>>> +/* Find unused worker slot. */
>>>>>> +if (!w->in_use)
>>>>>>  {
>>>>>> -worker = >workers[slot];
>>>>>> -break;
>>>>>> +worker = w;
>>>>>> +slot = i;
>>>>>> +}
>>>>>
>>>>> Doesn't this still need a break?  Otherwise it always picks the last slot.
>>>>>
>>>>
>>>> Yes it will pick the last slot, does that matter though, is the first
>>>> one better somehow?
>>>>
>>>> We can't break because we also need to continue the counter (I think the
>>>> issue that the counter solves is probably just theoretical, but still).
>>>
>>> I see.  I think the code would be less confusing if we break the loop
>>> like before and call logicalrep_sync_worker_count() separately.
>>>
>>>> Hmm actually, maybe the if (!w->in_use) should be if (worker == NULL &&
>>>> !w->in_use)?
>>>
>>> That would also do it.  But it's getting a bit fiddly.
>>>
>>
>> I just wanted to avoid looping twice, especially since the garbage
>> collecting code has to also do the loop. I guess I'll go with my
>> original coding for this then which was to put retry label above the
>> loop first, then try finding worker slot, if found call the
>> logicalrep_sync_worker_count and if not found do the garbage collection
>> and if we cleaned up something then goto retry.
>>
> 
> Here is the patch doing just that.
> 

And one more revision which also checks in_use when attaching shared
memory. This is mainly to improve the user visible behavior in
theoretical corner case when the worker is supposed to be cleaned up but
actually still manages to start (it would still fail even without this
change but the error message would be more obscure).

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


Fix-various-concurrency-issues-in-logical-replicatiov4.patch
Description: binary/octet-stream

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] PG 10 release notes

2017-04-25 Thread Petr Jelinek
On 25/04/17 17:01, Bruce Momjian wrote:
> 
>> I also wonder if ability to run SQL queries on walsender connected to a
>> database is worth mentioning (replication=database kind of connection).
> 
> Uh, why would that be important to users?

Because every tool that uses logical decoding to capture changes does no
need to open extra connection to fetch initial state.

(and we document much less user visible changes than this in the release
notes)

> 
>> Or the ability of logical decoding to follow timeline switches.
> 
> I didn't think logical decoding was really more than a proof-of-concept
> until now.
> 

Huh?

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] PG 10 release notes

2017-04-24 Thread Petr Jelinek
On 25/04/17 03:31, Bruce Momjian wrote:
> I have committed the first draft of the Postgres 10 release notes.  They
> are current as of two days ago, and I will keep them current.  Please
> give me any feedback you have.
> 

Cool!

> The only unusual thing is that this release has ~180 items while most
> recent release have had ~220.  The pattern I see that there are more
> large features in this release than previous ones.
> 

Well I think for example

> Optimizer
> 
> Add the ability to compute a correlation ratio and the number of distinct 
> values on several columns (Tomas Vondra, David Rowley)
> 

could be easily 2 or 3 items (explicitly defining additional statistics,
multicolumn ndistinct and functional dependencies).

I also wonder if ability to run SQL queries on walsender connected to a
database is worth mentioning (replication=database kind of connection).

Or the ability of logical decoding to follow timeline switches.

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


<    1   2   3   4   5   6   7   8   9   10   >