Horton

Thread

Commits

  1. Don't force-assign transaction id when exporting a snapshot.

  1. logical replication busy-waiting on a lock

    Jeff Janes <jeff.janes@gmail.com> — 2017-05-26T23:25:14Z

    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.

47462 idle in transaction 2017-05-26 16:05:20.505 PDT LOG:  logical
decoding found initial starting point at 1B/7BAC9D50
47462 idle in transaction 2017-05-26 16:05:20.505 PDT DETAIL:  Waiting for
transactions (approximately 9) older than 73326615 to end.
47462 idle in transaction waiting 2017-05-26 16:05:21.505 PDT LOG:  process
47462 still waiting for ShareLock on transaction 73322726 after 1000.060 ms
47462 idle in transaction waiting 2017-05-26 16:05:21.505 PDT DETAIL:
 Process holding the lock: 47457. Wait queue: 47462.
47462 idle in transaction waiting 2017-05-26 16:05:21.506 PDT LOG:  process
47462 still waiting for ShareLock on transaction 73322726 after 1000.398 ms
47462 idle in transaction waiting 2017-05-26 16:05:21.506 PDT DETAIL:
 Process holding the lock: 47457. Wait queue: 47462.
47462 idle in transaction waiting 2017-05-26 16:05:21.506 PDT LOG:  process
47462 still waiting for ShareLock on transaction 73322726 after 1000.574 ms
47462 idle in transaction waiting 2017-05-26 16:05:21.506 PDT DETAIL:
 Process holding the lock: 47457. Wait queue: 47462.
47462 idle in transaction waiting 2017-05-26 16:05:21.506 PDT LOG:  process
47462 still waiting for ShareLock on transaction 73322726 after 1000.816 ms
47462 idle in transaction waiting 2017-05-26 16:05:21.506 PDT DETAIL:
 Process holding the lock: 47457. Wait queue: 47462.
47462 idle in transaction waiting 2017-05-26 16:05:21.506 PDT LOG:  process
47462 still waiting for ShareLock on transaction 73322726 after 1001.180 ms
47462 idle in transaction waiting 2017-05-26 16:05:21.506 PDT DETAIL:
 Process holding the lock: 47457. Wait queue: 47462.
47462 idle in transaction waiting 2017-05-26 16:05:21.507 PDT LOG:  process
47462 still waiting for ShareLock on transaction 73322726 after 1001.284 ms
47462 idle in transaction waiting 2017-05-26 16:05:21.507 PDT DETAIL:
 Process holding the lock: 47457. Wait queue: 47462.
47462 idle in transaction waiting 2017-05-26 16:05:21.507 PDT LOG:  process
47462 still waiting for ShareLock on transaction 73322726 after 1001.493 ms
.....

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.

Cheers,

Jeff

  2. Re: logical replication busy-waiting on a lock

    Petr Jelinek <petr.jelinek@2ndquadrant.com> — 2017-05-27T13:44:05Z

    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

  3. Re: logical replication busy-waiting on a lock

    Petr Jelinek <petr.jelinek@2ndquadrant.com> — 2017-05-27T13:48:22Z

    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

  4. Re: logical replication busy-waiting on a lock

    Andres Freund <andres@anarazel.de> — 2017-05-27T15:17:19Z

    
On May 27, 2017 9:48:22 AM EDT, Petr Jelinek <petr.jelinek@2ndquadrant.com> wrote:
>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).

Hm.  I suspect the issue is that the exported snapshot needs an xid for some crosscheck, and that's what we're waiting for.  Could you check what happens if you don't assign one and just content the error checks out?   Not at my computer, just theorizing.

Andres

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

  5. Re: logical replication busy-waiting on a lock

    Jeff Janes <jeff.janes@gmail.com> — 2017-05-29T18:38:20Z

    On Sat, May 27, 2017 at 6:48 AM, Petr Jelinek <petr.jelinek@2ndquadrant.com>
wrote:

>
>
> 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].


Related, but not the same.  It would be nice if they didn't block, but if
they do have to block, shouldn't it wait on a semaphore, rather than doing
a tight loop?  It looks like maybe a latch didn't get reset when it should
have or something.


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


Cheers,

Jeff

  6. Re: logical replication busy-waiting on a lock

    Andres Freund <andres@anarazel.de> — 2017-05-29T18:40:26Z

    On 2017-05-29 11:38:20 -0700, Jeff Janes wrote:
> Related, but not the same.  It would be nice if they didn't block, but if
> they do have to block, shouldn't it wait on a semaphore, rather than doing
> a tight loop?  It looks like maybe a latch didn't get reset when it should
> have or something.

The code certainly is trying to just block using a lock (on the xid of
the running xact), there shouldn't be any busy looping going on...
There's no latch involved, so something is certainly weird here.

- Andres

  7. Re: logical replication busy-waiting on a lock

    Petr Jelinek <petr.jelinek@2ndquadrant.com> — 2017-05-29T18:58:05Z

    On 27/05/17 17:17, Andres Freund wrote:
> 
> 
> On May 27, 2017 9:48:22 AM EDT, Petr Jelinek <petr.jelinek@2ndquadrant.com> wrote:
>> 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).
> 
> Hm.  I suspect the issue is that the exported snapshot needs an xid for some crosscheck, and that's what we're waiting for.  Could you check what happens if you don't assign one and just content the error checks out?   Not at my computer, just theorizing.
> 

I don't think that's it, in my opinion it's the parallelization of table
data copy where we create snapshot for one process but then the next one
has to wait for the first one to finish. Before we fixed the
snapshotting, the second one would just use the ondisk snapshot so it
would work fine (except the snapshot was corrupted of course). I wonder
if we could somehow give it a hint to ignore the read-only txes, but
then we have no way to enforce the txes to stay read-only so it does not
seem safe.

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

  8. Re: logical replication busy-waiting on a lock

    Andres Freund <andres@anarazel.de> — 2017-05-29T18:59:28Z

    
On May 29, 2017 11:58:05 AM PDT, Petr Jelinek <petr.jelinek@2ndquadrant.com> wrote:
>On 27/05/17 17:17, Andres Freund wrote:
>> 
>> 
>> On May 27, 2017 9:48:22 AM EDT, Petr Jelinek
><petr.jelinek@2ndquadrant.com> wrote:
>>> 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).
>> 
>> Hm.  I suspect the issue is that the exported snapshot needs an xid
>for some crosscheck, and that's what we're waiting for.  Could you
>check what happens if you don't assign one and just content the error
>checks out?   Not at my computer, just theorizing.
>> 
>
>I don't think that's it, in my opinion it's the parallelization of
>table
>data copy where we create snapshot for one process but then the next
>one
>has to wait for the first one to finish. Before we fixed the
>snapshotting, the second one would just use the ondisk snapshot so it
>would work fine (except the snapshot was corrupted of course). I wonder
>if we could somehow give it a hint to ignore the read-only txes, but
>then we have no way to enforce the txes to stay read-only so it does
>not
>seem safe.

Read-only txs have no xid ...
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

  9. Re: logical replication busy-waiting on a lock

    Petr Jelinek <petr.jelinek@2ndquadrant.com> — 2017-05-29T19:21:50Z

    On 29/05/17 20:59, Andres Freund wrote:
> 
> 
> On May 29, 2017 11:58:05 AM PDT, Petr Jelinek <petr.jelinek@2ndquadrant.com> wrote:
>> On 27/05/17 17:17, Andres Freund wrote:
>>>
>>>
>>> On May 27, 2017 9:48:22 AM EDT, Petr Jelinek
>> <petr.jelinek@2ndquadrant.com> wrote:
>>>> 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).
>>>
>>> Hm.  I suspect the issue is that the exported snapshot needs an xid
>> for some crosscheck, and that's what we're waiting for.  Could you
>> check what happens if you don't assign one and just content the error
>> checks out?   Not at my computer, just theorizing.
>>>
>>
>> I don't think that's it, in my opinion it's the parallelization of
>> table
>> data copy where we create snapshot for one process but then the next
>> one
>> has to wait for the first one to finish. Before we fixed the
>> snapshotting, the second one would just use the ondisk snapshot so it
>> would work fine (except the snapshot was corrupted of course). I wonder
>> if we could somehow give it a hint to ignore the read-only txes, but
>> then we have no way to enforce the txes to stay read-only so it does
>> not
>> seem safe.
> 
> Read-only txs have no xid ...
> 

That's what I mean by hinting, normally they don't but building initial
snapshot in snapshot builder calls GetTopTransactionId() (see
SnapBuildInitialSnapshot()) which will assign it xid.

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

  10. Re: logical replication busy-waiting on a lock

    Andres Freund <andres@anarazel.de> — 2017-05-29T19:23:04Z

    
On May 29, 2017 12:21:50 PM PDT, Petr Jelinek <petr.jelinek@2ndquadrant.com> wrote:
>On 29/05/17 20:59, Andres Freund wrote:
>> 
>> 
>> On May 29, 2017 11:58:05 AM PDT, Petr Jelinek
><petr.jelinek@2ndquadrant.com> wrote:
>>> On 27/05/17 17:17, Andres Freund wrote:
>>>>
>>>>
>>>> On May 27, 2017 9:48:22 AM EDT, Petr Jelinek
>>> <petr.jelinek@2ndquadrant.com> wrote:
>>>>> 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).
>>>>
>>>> Hm.  I suspect the issue is that the exported snapshot needs an xid
>>> for some crosscheck, and that's what we're waiting for.  Could you
>>> check what happens if you don't assign one and just content the
>error
>>> checks out?   Not at my computer, just theorizing.
>>>>
>>>
>>> I don't think that's it, in my opinion it's the parallelization of
>>> table
>>> data copy where we create snapshot for one process but then the next
>>> one
>>> has to wait for the first one to finish. Before we fixed the
>>> snapshotting, the second one would just use the ondisk snapshot so
>it
>>> would work fine (except the snapshot was corrupted of course). I
>wonder
>>> if we could somehow give it a hint to ignore the read-only txes, but
>>> then we have no way to enforce the txes to stay read-only so it does
>>> not
>>> seem safe.
>> 
>> Read-only txs have no xid ...
>> 
>
>That's what I mean by hinting, normally they don't but building initial
>snapshot in snapshot builder calls GetTopTransactionId() (see
>SnapBuildInitialSnapshot()) which will assign it xid.

That's precisely what I pointed out a few emails above, and what I suggest changing.

Andres
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

  11. Re: logical replication busy-waiting on a lock

    Petr Jelinek <petr.jelinek@2ndquadrant.com> — 2017-05-29T19:25:35Z

    On 29/05/17 21:21, Petr Jelinek wrote:
> On 29/05/17 20:59, Andres Freund wrote:
>>
>>
>> On May 29, 2017 11:58:05 AM PDT, Petr Jelinek <petr.jelinek@2ndquadrant.com> wrote:
>>> On 27/05/17 17:17, Andres Freund wrote:
>>>>
>>>>
>>>> On May 27, 2017 9:48:22 AM EDT, Petr Jelinek
>>> <petr.jelinek@2ndquadrant.com> wrote:
>>>>> 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).
>>>>
>>>> Hm.  I suspect the issue is that the exported snapshot needs an xid
>>> for some crosscheck, and that's what we're waiting for.  Could you
>>> check what happens if you don't assign one and just content the error
>>> checks out?   Not at my computer, just theorizing.
>>>>
>>>
>>> I don't think that's it, in my opinion it's the parallelization of
>>> table
>>> data copy where we create snapshot for one process but then the next
>>> one
>>> has to wait for the first one to finish. Before we fixed the
>>> snapshotting, the second one would just use the ondisk snapshot so it
>>> would work fine (except the snapshot was corrupted of course). I wonder
>>> if we could somehow give it a hint to ignore the read-only txes, but
>>> then we have no way to enforce the txes to stay read-only so it does
>>> not
>>> seem safe.
>>
>> Read-only txs have no xid ...
>>
> 
> That's what I mean by hinting, normally they don't but building initial
> snapshot in snapshot builder calls GetTopTransactionId() (see
> SnapBuildInitialSnapshot()) which will assign it xid.
> 

Looking at the code more, the xid is only used as parameter for
SnapBuildBuildSnapshot() which never does anything with that parameter,
I wonder if it's really needed then.

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

  12. Re: logical replication busy-waiting on a lock

    Petr Jelinek <petr.jelinek@2ndquadrant.com> — 2017-05-29T19:27:47Z

    On 29/05/17 21:23, Andres Freund wrote:
> 
> 
> On May 29, 2017 12:21:50 PM PDT, Petr Jelinek <petr.jelinek@2ndquadrant.com> wrote:
>> On 29/05/17 20:59, Andres Freund wrote:
>>>
>>>
>>> On May 29, 2017 11:58:05 AM PDT, Petr Jelinek
>> <petr.jelinek@2ndquadrant.com> wrote:
>>>> On 27/05/17 17:17, Andres Freund wrote:
>>>>>
>>>>>
>>>>> On May 27, 2017 9:48:22 AM EDT, Petr Jelinek
>>>> <petr.jelinek@2ndquadrant.com> wrote:
>>>>>> 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).
>>>>>
>>>>> Hm.  I suspect the issue is that the exported snapshot needs an xid
>>>> for some crosscheck, and that's what we're waiting for.  Could you
>>>> check what happens if you don't assign one and just content the
>> error
>>>> checks out?   Not at my computer, just theorizing.
>>>>>
>>>>
>>>> I don't think that's it, in my opinion it's the parallelization of
>>>> table
>>>> data copy where we create snapshot for one process but then the next
>>>> one
>>>> has to wait for the first one to finish. Before we fixed the
>>>> snapshotting, the second one would just use the ondisk snapshot so
>> it
>>>> would work fine (except the snapshot was corrupted of course). I
>> wonder
>>>> if we could somehow give it a hint to ignore the read-only txes, but
>>>> then we have no way to enforce the txes to stay read-only so it does
>>>> not
>>>> seem safe.
>>>
>>> Read-only txs have no xid ...
>>>
>>
>> That's what I mean by hinting, normally they don't but building initial
>> snapshot in snapshot builder calls GetTopTransactionId() (see
>> SnapBuildInitialSnapshot()) which will assign it xid.
> 
> That's precisely what I pointed out a few emails above, and what I suggest changing.
> 

Ah didn't realize that's what you meant. I can try playing with it.

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

  13. Re: logical replication busy-waiting on a lock

    Andres Freund <andres@anarazel.de> — 2017-05-29T19:28:24Z

    
On May 29, 2017 12:25:35 PM PDT, Petr Jelinek <petr.jelinek@2ndquadrant.com> wrote:
>On 29/05/17 21:21, Petr Jelinek wrote:
>> On 29/05/17 20:59, Andres Freund wrote:
>>>
>>>
>>> On May 29, 2017 11:58:05 AM PDT, Petr Jelinek
><petr.jelinek@2ndquadrant.com> wrote:
>>>> On 27/05/17 17:17, Andres Freund wrote:
>>>>>
>>>>>
>>>>> On May 27, 2017 9:48:22 AM EDT, Petr Jelinek
>>>> <petr.jelinek@2ndquadrant.com> wrote:
>>>>>> 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).
>>>>>
>>>>> Hm.  I suspect the issue is that the exported snapshot needs an
>xid
>>>> for some crosscheck, and that's what we're waiting for.  Could you
>>>> check what happens if you don't assign one and just content the
>error
>>>> checks out?   Not at my computer, just theorizing.
>>>>>
>>>>
>>>> I don't think that's it, in my opinion it's the parallelization of
>>>> table
>>>> data copy where we create snapshot for one process but then the
>next
>>>> one
>>>> has to wait for the first one to finish. Before we fixed the
>>>> snapshotting, the second one would just use the ondisk snapshot so
>it
>>>> would work fine (except the snapshot was corrupted of course). I
>wonder
>>>> if we could somehow give it a hint to ignore the read-only txes,
>but
>>>> then we have no way to enforce the txes to stay read-only so it
>does
>>>> not
>>>> seem safe.
>>>
>>> Read-only txs have no xid ...
>>>
>> 
>> That's what I mean by hinting, normally they don't but building
>initial
>> snapshot in snapshot builder calls GetTopTransactionId() (see
>> SnapBuildInitialSnapshot()) which will assign it xid.
>> 
>
>Looking at the code more, the xid is only used as parameter for
>SnapBuildBuildSnapshot() which never does anything with that parameter,
>I wonder if it's really needed then.

Not at a computer, but by memory that'll trigger the snapshot export routine to include it.  Import in turn requires the xid to check if the source is still alive.  But there's better ways, e.g. using the virtual xactid.

Andres
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

  14. Re: logical replication busy-waiting on a lock

    Petr Jelinek <petr.jelinek@2ndquadrant.com> — 2017-05-29T19:41:26Z

    On 29/05/17 21:28, Andres Freund wrote:
> 
> On May 29, 2017 12:25:35 PM PDT, Petr Jelinek <petr.jelinek@2ndquadrant.com> wrote:
>>
>> Looking at the code more, the xid is only used as parameter for
>> SnapBuildBuildSnapshot() which never does anything with that parameter,
>> I wonder if it's really needed then.
> 
> Not at a computer, but by memory that'll trigger the snapshot export routine to include it.  Import in turn requires the xid to check if the source is still alive.  But there's better ways, e.g. using the virtual xactid.
> 

It does, and while that's unfortunate the logical replication does not
actually export the snapshots. It uses the USE_SNAPSHOT option where the
snapshot is just installed into current transaction but not exported. So
not calling the GetTopTransactionId() would solve it at least for that
in-core use-case. I don't see any bad side effects from doing so yet, so
it might be good enough solution for PG10.

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

  15. Re: logical replication busy-waiting on a lock

    Andres Freund <andres@anarazel.de> — 2017-05-29T19:44:52Z

    
On May 29, 2017 12:41:26 PM PDT, Petr Jelinek <petr.jelinek@2ndquadrant.com> wrote:
>On 29/05/17 21:28, Andres Freund wrote:
>> 
>> On May 29, 2017 12:25:35 PM PDT, Petr Jelinek
><petr.jelinek@2ndquadrant.com> wrote:
>>>
>>> Looking at the code more, the xid is only used as parameter for
>>> SnapBuildBuildSnapshot() which never does anything with that
>parameter,
>>> I wonder if it's really needed then.
>> 
>> Not at a computer, but by memory that'll trigger the snapshot export
>routine to include it.  Import in turn requires the xid to check if the
>source is still alive.  But there's better ways, e.g. using the virtual
>xactid.
>> 
>
>It does, and while that's unfortunate the logical replication does not
>actually export the snapshots. It uses the USE_SNAPSHOT option where
>the
>snapshot is just installed into current transaction but not exported.
>So
>not calling the GetTopTransactionId() would solve it at least for that
>in-core use-case. I don't see any bad side effects from doing so yet,
>so
>it might be good enough solution for PG10.

In the general case you can't do so, because of vacuum and such.  Even for LR we need to make sure the exporting session didn't die badly, deleting the slot.  Hence suggestion to use the virtual xid.

Andres
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

  16. Re: logical replication busy-waiting on a lock

    Petr Jelinek <petr.jelinek@2ndquadrant.com> — 2017-05-29T21:49:33Z

    On 29/05/17 21:44, Andres Freund wrote:
> 
> 
> On May 29, 2017 12:41:26 PM PDT, Petr Jelinek <petr.jelinek@2ndquadrant.com> wrote:
>> On 29/05/17 21:28, Andres Freund wrote:
>>>
>>> On May 29, 2017 12:25:35 PM PDT, Petr Jelinek
>> <petr.jelinek@2ndquadrant.com> wrote:
>>>>
>>>> Looking at the code more, the xid is only used as parameter for
>>>> SnapBuildBuildSnapshot() which never does anything with that
>> parameter,
>>>> I wonder if it's really needed then.
>>>
>>> Not at a computer, but by memory that'll trigger the snapshot export
>> routine to include it.  Import in turn requires the xid to check if the
>> source is still alive.  But there's better ways, e.g. using the virtual
>> xactid.
>>>
>>
>> It does, and while that's unfortunate the logical replication does not
>> actually export the snapshots. It uses the USE_SNAPSHOT option where
>> the
>> snapshot is just installed into current transaction but not exported.
>> So
>> not calling the GetTopTransactionId() would solve it at least for that
>> in-core use-case. I don't see any bad side effects from doing so yet,
>> so
>> it might be good enough solution for PG10.
> 
> In the general case you can't do so, because of vacuum and such.  Even for LR we need to make sure the exporting session didn't die badly, deleting the slot.  Hence suggestion to use the virtual xid.
> 

I am not quite sure I understand (both the vxid suggestion and for the
session dying badly). Maybe we can discuss bit more when you get to
computer so it's easier for you to expand on what you mean.

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

  17. Re: logical replication busy-waiting on a lock

    Andres Freund <andres@anarazel.de> — 2017-05-31T07:24:48Z

    On 2017-05-29 23:49:33 +0200, Petr Jelinek wrote:
> I am not quite sure I understand (both the vxid suggestion and for the
> session dying badly). Maybe we can discuss bit more when you get to
> computer so it's easier for you to expand on what you mean.

The xid interlock when exporting a snapshot is required because
otherwise it's not generally guaranteed that all resourced required for
the snapshot are reserved.  In the logical replication case we could
guarantee that otherwise, but there'd be weird-ish edge cases when
erroring out just after exporting a snapshot.

The problem with using the xid as that interlock is that it requires
acquiring an xid - which is something we're going to block against when
building a new catalog snapshot.  Afaict that's not entirely required -
all that we need to verify is that the snapshot in the source
transaction is still running.  The easiest way for the importer to check
that the source is still alive seems to be export the virtual
transaction id instead of the xid.  Normally we can't store things like
virtual xids on disk, but that concern isn't valid here because exported
snapshots are ephemeral, there's also already a precedent in
predicate.c.

It seems like it'd be fairly easy to change things around that way, but
maybe I'm missing something.


- Andres

  18. Re: logical replication busy-waiting on a lock

    Petr Jelinek <petr.jelinek@2ndquadrant.com> — 2017-05-31T09:21:56Z

    On 31/05/17 09:24, Andres Freund wrote:
> On 2017-05-29 23:49:33 +0200, Petr Jelinek wrote:
>> I am not quite sure I understand (both the vxid suggestion and for the
>> session dying badly). Maybe we can discuss bit more when you get to
>> computer so it's easier for you to expand on what you mean.
> 
> The xid interlock when exporting a snapshot is required because
> otherwise it's not generally guaranteed that all resourced required for
> the snapshot are reserved.  In the logical replication case we could
> guarantee that otherwise, but there'd be weird-ish edge cases when
> erroring out just after exporting a snapshot.
> 
> The problem with using the xid as that interlock is that it requires
> acquiring an xid - which is something we're going to block against when
> building a new catalog snapshot.  Afaict that's not entirely required -
> all that we need to verify is that the snapshot in the source
> transaction is still running.  The easiest way for the importer to check
> that the source is still alive seems to be export the virtual
> transaction id instead of the xid.  Normally we can't store things like
> virtual xids on disk, but that concern isn't valid here because exported
> snapshots are ephemeral, there's also already a precedent in
> predicate.c.
> 
> It seems like it'd be fairly easy to change things around that way, but
> maybe I'm missing something.
> 

Okay, thanks for explanation. Code-wise it does seem simple enough
indeed. I admit I don't know enough about the exported snapshots and
snapshot management in general to be able to answer the question of
safety here. That said, it does seem to me like it should work as the
exported snapshots are just on disk representation of in-memory state
that becomes invalid once the in-memory state does.

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

  19. Re: logical replication busy-waiting on a lock

    Petr Jelinek <petr.jelinek@2ndquadrant.com> — 2017-06-01T12:17:44Z

    On 31/05/17 11:21, Petr Jelinek wrote:
> On 31/05/17 09:24, Andres Freund wrote:
>> On 2017-05-29 23:49:33 +0200, Petr Jelinek wrote:
>>> I am not quite sure I understand (both the vxid suggestion and for the
>>> session dying badly). Maybe we can discuss bit more when you get to
>>> computer so it's easier for you to expand on what you mean.
>>
>> The xid interlock when exporting a snapshot is required because
>> otherwise it's not generally guaranteed that all resourced required for
>> the snapshot are reserved.  In the logical replication case we could
>> guarantee that otherwise, but there'd be weird-ish edge cases when
>> erroring out just after exporting a snapshot.
>>
>> The problem with using the xid as that interlock is that it requires
>> acquiring an xid - which is something we're going to block against when
>> building a new catalog snapshot.  Afaict that's not entirely required -
>> all that we need to verify is that the snapshot in the source
>> transaction is still running.  The easiest way for the importer to check
>> that the source is still alive seems to be export the virtual
>> transaction id instead of the xid.  Normally we can't store things like
>> virtual xids on disk, but that concern isn't valid here because exported
>> snapshots are ephemeral, there's also already a precedent in
>> predicate.c.
>>
>> It seems like it'd be fairly easy to change things around that way, but
>> maybe I'm missing something.
>>
> 
> Okay, thanks for explanation. Code-wise it does seem simple enough
> indeed. I admit I don't know enough about the exported snapshots and
> snapshot management in general to be able to answer the question of
> safety here. That said, it does seem to me like it should work as the
> exported snapshots are just on disk representation of in-memory state
> that becomes invalid once the in-memory state does.
> 

Thinking more about this, I am not convinced it's a good idea to change
exports this late in the cycle. I still think it's best to do the xid
assignment only when the snapshot is actually exported but don't assign
xid when the export is only used by the local session (the new option in
PG10). That's one line change which impacts only logical
replication/decoding as opposed to everything else which uses exported
snapshots.

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

  20. Re: logical replication busy-waiting on a lock

    Andres Freund <andres@anarazel.de> — 2017-06-01T18:28:46Z

    On 2017-06-01 14:17:44 +0200, Petr Jelinek wrote:
> Thinking more about this, I am not convinced it's a good idea to change
> exports this late in the cycle. I still think it's best to do the xid
> assignment only when the snapshot is actually exported but don't assign
> xid when the export is only used by the local session (the new option in
> PG10). That's one line change which impacts only logical
> replication/decoding as opposed to everything else which uses exported
> snapshots.

I'm not quite convinced by this argument.  Exported snapshot contents
are ephemeral, we can change the format at any time.  The wait is fairly
annoying for every user of logical decoding.  For me the combination of
those two fact implies that we should rather fix this properly.

- Andres

  21. Re: logical replication busy-waiting on a lock

    Robert Haas <robertmhaas@gmail.com> — 2017-06-02T01:38:41Z

    On Thu, Jun 1, 2017 at 2:28 PM, Andres Freund <andres@anarazel.de> wrote:
> On 2017-06-01 14:17:44 +0200, Petr Jelinek wrote:
>> Thinking more about this, I am not convinced it's a good idea to change
>> exports this late in the cycle. I still think it's best to do the xid
>> assignment only when the snapshot is actually exported but don't assign
>> xid when the export is only used by the local session (the new option in
>> PG10). That's one line change which impacts only logical
>> replication/decoding as opposed to everything else which uses exported
>> snapshots.
>
> I'm not quite convinced by this argument.  Exported snapshot contents
> are ephemeral, we can change the format at any time.  The wait is fairly
> annoying for every user of logical decoding.  For me the combination of
> those two fact implies that we should rather fix this properly.

+1.  The change Andres is proposing doesn't sound like it will be
terribly high-impact, and I think we'll be happier in the long run if
we install real fixes instead of kludging it.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  22. Re: logical replication busy-waiting on a lock

    Petr Jelinek <petr.jelinek@2ndquadrant.com> — 2017-06-03T01:03:20Z

    On 02/06/17 03:38, Robert Haas wrote:
> On Thu, Jun 1, 2017 at 2:28 PM, Andres Freund <andres@anarazel.de> wrote:
>> On 2017-06-01 14:17:44 +0200, Petr Jelinek wrote:
>>> Thinking more about this, I am not convinced it's a good idea to change
>>> exports this late in the cycle. I still think it's best to do the xid
>>> assignment only when the snapshot is actually exported but don't assign
>>> xid when the export is only used by the local session (the new option in
>>> PG10). That's one line change which impacts only logical
>>> replication/decoding as opposed to everything else which uses exported
>>> snapshots.
>>
>> I'm not quite convinced by this argument.  Exported snapshot contents
>> are ephemeral, we can change the format at any time.  The wait is fairly
>> annoying for every user of logical decoding.  For me the combination of
>> those two fact implies that we should rather fix this properly.
> 
> +1.  The change Andres is proposing doesn't sound like it will be
> terribly high-impact, and I think we'll be happier in the long run if
> we install real fixes instead of kludging it.
> 

All right, here is my rough attempt on doing what Andres has suggested,
going straight from xid to vxid, seems to work fine in my tests and
parallel pg_dump seems happy with it as well.

The second patch removes the xid parameter from SnapBuildBuildSnapshot
as it's not used for anything and skips the GetTopTransactionId() call
as well. That should solve the original problem reported here.

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

  23. Re: logical replication busy-waiting on a lock

    Andres Freund <andres@anarazel.de> — 2017-06-03T01:25:48Z

    Hi,

On 2017-06-03 03:03:20 +0200, Petr Jelinek wrote:
> All right, here is my rough attempt on doing what Andres has suggested,
> going straight from xid to vxid, seems to work fine in my tests and
> parallel pg_dump seems happy with it as well.

Cool!


> The second patch removes the xid parameter from SnapBuildBuildSnapshot
> as it's not used for anything and skips the GetTopTransactionId() call
> as well.

Makes sense.


> That should solve the original problem reported here.

Did you verify that?


> From 4f7eb5b8fdbab2539731414e81d7a7419a83e5b1 Mon Sep 17 00:00:00 2001
> From: Petr Jelinek <pjmodos@pjmodos.net>
> Date: Sat, 3 Jun 2017 02:06:22 +0200
> Subject: [PATCH 1/2] Use virtual transaction instead of normal ones in
>  exported snapshots

> @@ -1817,8 +1818,10 @@ ProcArrayInstallImportedXmin(TransactionId xmin, TransactionId sourcexid)
>  		if (pgxact->vacuumFlags & PROC_IN_VACUUM)
>  			continue;
>  
> -		xid = pgxact->xid;		/* fetch just once */

Hm, I don't quite understand what the 'fetch just once' bit was trying
to achieve here (not to speak of the fact that it's simply not
guaranteed this way).  Therefore I think you're right to simply
disregard it.


> @@ -1741,17 +1741,17 @@ GetSerializableTransactionSnapshotInt(Snapshot snapshot,
>  	} while (!sxact);
>  
>  	/* Get the snapshot, or check that it's safe to use */
> -	if (!TransactionIdIsValid(sourcexid))
> +	if (!sourcevxid)
>  		snapshot = GetSnapshotData(snapshot);
> -	else if (!ProcArrayInstallImportedXmin(snapshot->xmin, sourcexid))
> +	else if (!ProcArrayInstallImportedXmin(snapshot->xmin, sourcevxid))
>  	{
>  		ReleasePredXact(sxact);
>  		LWLockRelease(SerializableXactHashLock);
>  		ereport(ERROR,
>  				(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
>  				 errmsg("could not import the requested snapshot"),
> -			   errdetail("The source transaction %u is not running anymore.",
> -						 sourcexid)));
> +	   errdetail("The source virtual transaction %d/%u is not running anymore.",
> +					 sourcevxid->backendId, sourcevxid->localTransactionId)));

Hm, this is a harder to read.  Wonder if we should add a pid field, that'd
make it a bit easier to interpret?


- Andres

  24. Re: logical replication busy-waiting on a lock

    Petr Jelinek <petr.jelinek@2ndquadrant.com> — 2017-06-03T02:50:00Z

    On 03/06/17 03:25, Andres Freund wrote:
> 
>> That should solve the original problem reported here.
> 
> Did you verify that?
>

Yes, I tried to manually create multiple exported logical decoding
snapshots in parallel and read data from them and it worked fine, while
it blocked before.

> 
>> @@ -1741,17 +1741,17 @@ GetSerializableTransactionSnapshotInt(Snapshot snapshot,
>>  	} while (!sxact);
>>  
>>  	/* Get the snapshot, or check that it's safe to use */
>> -	if (!TransactionIdIsValid(sourcexid))
>> +	if (!sourcevxid)
>>  		snapshot = GetSnapshotData(snapshot);
>> -	else if (!ProcArrayInstallImportedXmin(snapshot->xmin, sourcexid))
>> +	else if (!ProcArrayInstallImportedXmin(snapshot->xmin, sourcevxid))
>>  	{
>>  		ReleasePredXact(sxact);
>>  		LWLockRelease(SerializableXactHashLock);
>>  		ereport(ERROR,
>>  				(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
>>  				 errmsg("could not import the requested snapshot"),
>> -			   errdetail("The source transaction %u is not running anymore.",
>> -						 sourcexid)));
>> +	   errdetail("The source virtual transaction %d/%u is not running anymore.",
>> +					 sourcevxid->backendId, sourcevxid->localTransactionId)));
> 
> Hm, this is a harder to read.  Wonder if we should add a pid field, that'd
> make it a bit easier to interpret?
> 

Agreed, see attached. We have to pass the pid around a bit but I don't
think it's too bad.

One thing I don't like is the GetLastLocalTransactionId() that I had to
add because we clear the proc->lxid before we get to AtEOXact_Snapshot()
but I don't have better solution there.

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

  25. Re: logical replication busy-waiting on a lock

    Andres Freund <andres@anarazel.de> — 2017-06-08T23:06:50Z

    Hi,

On 2017-06-03 04:50:00 +0200, Petr Jelinek wrote:
> One thing I don't like is the GetLastLocalTransactionId() that I had to
> add because we clear the proc->lxid before we get to AtEOXact_Snapshot()
> but I don't have better solution there.

I dislike that quite a bit - how about we instead just change the
information we keep in exportedSnapshots?  I.e. store a pointer to an
struct ExportedSnapshot
{
    char *exportedAs;
    Snapshot snapshot;
}

Then we can get rid of that relatively ugly list_length() business too.

- Andres

  26. Re: logical replication busy-waiting on a lock

    Petr Jelinek <petr.jelinek@2ndquadrant.com> — 2017-06-09T01:20:03Z

    On 09/06/17 01:06, Andres Freund wrote:
> Hi,
> 
> On 2017-06-03 04:50:00 +0200, Petr Jelinek wrote:
>> One thing I don't like is the GetLastLocalTransactionId() that I had to
>> add because we clear the proc->lxid before we get to AtEOXact_Snapshot()
>> but I don't have better solution there.
> 
> I dislike that quite a bit - how about we instead just change the
> information we keep in exportedSnapshots?  I.e. store a pointer to an
> struct ExportedSnapshot
> {
>     char *exportedAs;
>     Snapshot snapshot;
> }
> 
> Then we can get rid of that relatively ugly list_length() business too.
> 

That sounds reasonable, I'll do that.

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

  27. Re: logical replication busy-waiting on a lock

    Petr Jelinek <petr.jelinek@2ndquadrant.com> — 2017-06-09T20:28:00Z

    On 09/06/17 03:20, Petr Jelinek wrote:
> On 09/06/17 01:06, Andres Freund wrote:
>> Hi,
>>
>> On 2017-06-03 04:50:00 +0200, Petr Jelinek wrote:
>>> One thing I don't like is the GetLastLocalTransactionId() that I had to
>>> add because we clear the proc->lxid before we get to AtEOXact_Snapshot()
>>> but I don't have better solution there.
>>
>> I dislike that quite a bit - how about we instead just change the
>> information we keep in exportedSnapshots?  I.e. store a pointer to an
>> struct ExportedSnapshot
>> {
>>     char *exportedAs;
>>     Snapshot snapshot;
>> }
>>
>> Then we can get rid of that relatively ugly list_length() business too.
>>
> 
> That sounds reasonable, I'll do that.
> 

And here it is, seems better (the 0002 is same as before).

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

  28. Re: logical replication busy-waiting on a lock

    Andres Freund <andres@anarazel.de> — 2017-06-09T20:31:55Z

    On 2017-06-09 22:28:00 +0200, Petr Jelinek wrote:
> And here it is, seems better (the 0002 is same as before).

Cool, looks good on a quick scan.


>  /* Define pathname of exported-snapshot files */
>  #define SNAPSHOT_EXPORT_DIR "pg_snapshots"
> -#define XactExportFilePath(path, xid, num, suffix) \
> -	snprintf(path, sizeof(path), SNAPSHOT_EXPORT_DIR "/%08X-%d%s", \
> -			 xid, num, suffix)
>  
> -/* Current xact's exported snapshots (a list of Snapshot structs) */
> +/* Structure holding info about exported snapshot. */
> +typedef struct ExportedSnapshot
> +{
> +	char *snapfile;
> +	Snapshot snapshot;
> +} ExportedSnapshot;
> +
> +/* Current xact's exported snapshots (a list of ExportedSnapshot structs) */
>  static List *exportedSnapshots = NIL;

trival quibble: *pointers to


Will take care of it over the weekend.

Greetings,

Andres Freund

  29. Re: logical replication busy-waiting on a lock

    Andres Freund <andres@anarazel.de> — 2017-06-13T07:32:40Z

    On 2017-06-09 22:28:00 +0200, Petr Jelinek wrote:
> On 09/06/17 03:20, Petr Jelinek wrote:
> > On 09/06/17 01:06, Andres Freund wrote:
> >> Hi,
> >>
> >> On 2017-06-03 04:50:00 +0200, Petr Jelinek wrote:
> >>> One thing I don't like is the GetLastLocalTransactionId() that I had to
> >>> add because we clear the proc->lxid before we get to AtEOXact_Snapshot()
> >>> but I don't have better solution there.
> >>
> >> I dislike that quite a bit - how about we instead just change the
> >> information we keep in exportedSnapshots?  I.e. store a pointer to an
> >> struct ExportedSnapshot
> >> {
> >>     char *exportedAs;
> >>     Snapshot snapshot;
> >> }
> >>
> >> Then we can get rid of that relatively ugly list_length() business too.
> >>
> > 
> > That sounds reasonable, I'll do that.
> > 
> 
> And here it is, seems better (the 0002 is same as before).

I spent a chunk of time with this.  One surprising, but easy to fix issue was
that this patch had the exported file name as:

>  	/*
> +	 * Generate file path for the snapshot.  We start numbering of snapshots
> +	 * inside the transaction from 1.
> +	 */
> +	snprintf(path, sizeof(path), SNAPSHOT_EXPORT_DIR "/%08X-%d",
> +			 topXid, list_length(exportedSnapshots) + 1);
> +

I.e. just as before, unlike the previous version of the patch which used
virtualxids.  Given that we don't require topXid to be set, this seems
to largely break the patch.

Secondly, because of

>  	/*
> -	 * This will assign a transaction ID if we do not yet have one.
> +	 * Get our transaction ID if there is one, to include in the snapshot.
>  	 */
> -	topXid = GetTopTransactionId();
> +	topXid = GetTopTransactionIdIfAny();
>  

which is perfectly correct on its face, we actually added a substantial
feature: Previously exporting snapshots was only possible on primaries,
now it's also possible on standbys.  The only thing that made that error
out before was the check during xid assignment.  Because snapshots work
somewhat differntly on standbys, I spent a good chunk of time staring at
code trying to see whether it'd break anything.  Neither code-reading
nor testing seems to suggest any problems.   Were we earlier in the
release cycle I'd be perfectly fine with an accidental feature, but I'm
wondering a bit whether we should just make it error out at this point,
because it'd be a new feature.  I'm -0.5 on that, but I think it should
be raised.

- Andres

  30. Re: logical replication busy-waiting on a lock

    Andres Freund <andres@anarazel.de> — 2017-06-13T07:50:20Z

    On 2017-06-13 00:32:40 -0700, Andres Freund wrote:
> On 2017-06-09 22:28:00 +0200, Petr Jelinek wrote:
> > On 09/06/17 03:20, Petr Jelinek wrote:
> > > On 09/06/17 01:06, Andres Freund wrote:
> > >> Hi,
> > >>
> > >> On 2017-06-03 04:50:00 +0200, Petr Jelinek wrote:
> > >>> One thing I don't like is the GetLastLocalTransactionId() that I had to
> > >>> add because we clear the proc->lxid before we get to AtEOXact_Snapshot()
> > >>> but I don't have better solution there.
> > >>
> > >> I dislike that quite a bit - how about we instead just change the
> > >> information we keep in exportedSnapshots?  I.e. store a pointer to an
> > >> struct ExportedSnapshot
> > >> {
> > >>     char *exportedAs;
> > >>     Snapshot snapshot;
> > >> }
> > >>
> > >> Then we can get rid of that relatively ugly list_length() business too.
> > >>
> > > 
> > > That sounds reasonable, I'll do that.
> > > 
> > 
> > And here it is, seems better (the 0002 is same as before).
> 
> I spent a chunk of time with this.  One surprising, but easy to fix issue was
> that this patch had the exported file name as:
> 
> >  	/*
> > +	 * Generate file path for the snapshot.  We start numbering of snapshots
> > +	 * inside the transaction from 1.
> > +	 */
> > +	snprintf(path, sizeof(path), SNAPSHOT_EXPORT_DIR "/%08X-%d",
> > +			 topXid, list_length(exportedSnapshots) + 1);
> > +
> 
> I.e. just as before, unlike the previous version of the patch which used
> virtualxids.  Given that we don't require topXid to be set, this seems
> to largely break the patch.
> 
> Secondly, because of
> 
> >  	/*
> > -	 * This will assign a transaction ID if we do not yet have one.
> > +	 * Get our transaction ID if there is one, to include in the snapshot.
> >  	 */
> > -	topXid = GetTopTransactionId();
> > +	topXid = GetTopTransactionIdIfAny();
> >  
> 
> which is perfectly correct on its face, we actually added a substantial
> feature: Previously exporting snapshots was only possible on primaries,
> now it's also possible on standbys.  The only thing that made that error
> out before was the check during xid assignment.  Because snapshots work
> somewhat differntly on standbys, I spent a good chunk of time staring at
> code trying to see whether it'd break anything.  Neither code-reading
> nor testing seems to suggest any problems.   Were we earlier in the
> release cycle I'd be perfectly fine with an accidental feature, but I'm
> wondering a bit whether we should just make it error out at this point,
> because it'd be a new feature.  I'm -0.5 on that, but I think it should
> be raised.


Just to be clear: The patch, after the first point above (which I did),
looks ok.  I'm just looking for comments.

- Andres

  31. Re: logical replication busy-waiting on a lock

    Andres Freund <andres@anarazel.de> — 2017-06-14T18:57:56Z

    Hi,

On 2017-06-13 00:50:20 -0700, Andres Freund wrote:
> Just to be clear: The patch, after the first point above (which I did),
> looks ok.  I'm just looking for comments.

And pushed.

- Andres

  32. Re: logical replication busy-waiting on a lock

    Petr Jelinek <petr.jelinek@2ndquadrant.com> — 2017-06-14T22:35:51Z

    On 14/06/17 20:57, Andres Freund wrote:
> Hi,
> 
> On 2017-06-13 00:50:20 -0700, Andres Freund wrote:
>> Just to be clear: The patch, after the first point above (which I did),
>> looks ok.  I'm just looking for comments.
> 
> And pushed.
> 

Thanks!

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