Thread: [HACKERS] logical replication busy-waiting on a lock
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
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
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
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, justtheorizing. Andres -- Sent from my Android device with K-9 Mail. Please excuse my brevity.
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
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
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 waitingfor. 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
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.
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
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.
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
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
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 thexid 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.
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 thexid 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
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 sessiondidn'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.
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 sessiondidn'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
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
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
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
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
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
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
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
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
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
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
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
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
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
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
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
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