Fix slot's xmin advancement and subxact's lost snapshots in decoding.

(delicate ping)

On Sun, Apr 08, 2018 at 08:46:04AM +0300, Arseny Sher wrote:

I've discovered a couple of bugs in logical decoding code, both leading
to incorrect decoding results in somewhat rare cases. First, xmin of
slots is advanced too early. This affects the results only when
interlocking allows to perform DDL concurrently with looking at the
schema. In fact, I was not aware about such DDL until at
/messages/by-id/87tvu0p0jm.fsf@ars-thinkpad
I raised this question and Andres pointed out ALTER of composite
types. Probably there are others, I am not sure; it would be interesting
to know them.

Another problem is that new snapshots are never queued to known
subxacts. It means decoding results can be wrong if toplevel doesn't
write anything while subxact does.

Most people are recovering from the last commit fest, where many patches
have been discussed and dealt with. I have not looked in details at
your patch so I cannot say is legit or not, but for now I would
recommend to register this patch to the next commit fest under the
category "Bug Fixes" so as it does not fall into the cracks:
https://commitfest.postgresql.org/18/

I have added an entry to the open items in the section for older bugs:
https://wiki.postgresql.org/wiki/PostgreSQL_11_Open_Items#Older_Bugs
However this list tends to be... Er... Ignored.

Please see detailed description of the issues, tests which reproduce
them and fixes in the attached patch.

Those are always good things to have in a patch.
--
Michael

Michael Paquier <michael@paquier.xyz> writes:

but for now I would recommend to register this patch to the next
commit fest under the category "Bug Fixes" so as it does not fall into
the cracks: https://commitfest.postgresql.org/18/

I have added an entry to the open items in the section for older bugs:
https://wiki.postgresql.org/wiki/PostgreSQL_11_Open_Items#Older_Bugs
However this list tends to be... Er... Ignored.

Thank you, Michael. I have created the commitfest entry.

--
Arseny Sher
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Arseny Sher <a.sher@postgrespro.ru> wrote:

Please see detailed description of the issues, tests which reproduce
them and fixes in the attached patch.

I've played with your tests and gdb for a while, both w/o and with your
patch. I think I can understand both problems. I could not invent simpler way
to fix them.

One comment about the coding: since you introduce a new transaction list and
it's sorted by LSN, I think you should make the function AssertTXNLsnOrder()
more generic and use it to check the by_base_snapshot_lsn list too. For
example call it after the list insertion and deletion in
ReorderBufferPassBaseSnapshot().

Also I think it makes no harm if you split the diff into two, although both
fixes use the by_base_snapshot_lsn field.

--
Antonin Houska
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26, A-2700 Wiener Neustadt
Web: https://www.cybertec-postgresql.com

On 2018-Jun-11, Antonin Houska wrote:

Arseny Sher <a.sher@postgrespro.ru> wrote:

Please see detailed description of the issues, tests which reproduce
them and fixes in the attached patch.

I've played with your tests and gdb for a while, both w/o and with your
patch. I think I can understand both problems. I could not invent simpler way
to fix them.

One comment about the coding: since you introduce a new transaction list and
it's sorted by LSN, I think you should make the function AssertTXNLsnOrder()
more generic and use it to check the by_base_snapshot_lsn list too. For
example call it after the list insertion and deletion in
ReorderBufferPassBaseSnapshot().

I've been going over this patch also, and I've made a few minor edits of
the patch and the existing code as I come to understand what it's all
about.

Also I think it makes no harm if you split the diff into two, although both
fixes use the by_base_snapshot_lsn field.

Please don't.

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Hello,

Thank you for your time.

Alvaro Herrera <alvherre@2ndquadrant.com> writes:

On 2018-Jun-11, Antonin Houska wrote:

One comment about the coding: since you introduce a new transaction list and
it's sorted by LSN, I think you should make the function AssertTXNLsnOrder()
more generic and use it to check the by_base_snapshot_lsn list too. For
example call it after the list insertion and deletion in
ReorderBufferPassBaseSnapshot().

Ok, probably this makes sense. Updated patch is attached.

Also I think it makes no harm if you split the diff into two, although both
fixes use the by_base_snapshot_lsn field.

Please don't.

Yeah, I don't think we should bother with that.

--
Arseny Sher
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Hello

Firstly -- this is top-notch detective work, kudos and thanks for the
patch and test cases. (I verified that both fail before the code fix.)

Here's a v3. I applied a lot of makeup in order to try to understand
what's going on. I *think* I have a grasp on the original code and your
bugfix, not terribly firm I admit.

Some comments

* you don't need to Assert that things are not NULL if you're
immediately going to dereference them. The assert is there to make
the code crash in case it's a NULL pointer, but the subsequent
dereference is going to have the same effect, so the assert is
redundant.

* I think setting snapshot_base to NULL in ReorderBufferCleanupTXN is
pointless, since the struct is gonna be freed shortly afterwards.

* I rewrote many comments (both existing and some of the ones your patch
adds), and added lots of comments where there were none.

* I'm a bit unsure about the comment atop ReorderBufferSetBaseSnapshot.
Obviously, the bit within the #if 0/#endif I'm going to remove before
push. I don't understand why it says "Needs to be called before any
changes are added with ReorderBufferQueueChange"; but if you edit that
function and add an assert that the base snapshot is set, it crashes
pretty quickly in the test_decoding tests. (The supposedly bogus
comment was there before your patch -- I'm not saying your comment
addition is faulty.)

* I also noticed that we're doing subtxn cleanup one by one in both
ReorderBufferAssignChild and ReorderBufferCommitChild, which means the
top-level txn is sought in the hash table over and over, which seems a
bit silly. Not this patch's problem to fix ...

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Alvaro Herrera <alvherre@2ndquadrant.com> writes:

Firstly -- this is top-notch detective work, kudos and thanks for the
patch and test cases. (I verified that both fail before the code fix.)

Thank you!

Here's a v3. I applied a lot of makeup in order to try to understand
what's going on. I *think* I have a grasp on the original code and your
bugfix, not terribly firm I admit.

Well, when I started digging it, I have found logical decoding code
somewhat underdocumented. If you or any other committer will consider
the patch trying to improve the comments, I can prepare one.

* you don't need to Assert that things are not NULL if you're
immediately going to dereference them.

Indeed.

* I think setting snapshot_base to NULL in ReorderBufferCleanupTXN is
pointless, since the struct is gonna be freed shortly afterwards.

Yeah, but it is a general style of the original code, e.g. see
/* cosmetic... */
comments. It probably makes the picture a bit more aesthetic and
consistent, kind of final chord, though I don't mind removing it.

* I rewrote many comments (both existing and some of the ones your patch
adds), and added lots of comments where there were none.

Now the code is nicer, thanks.

* I'm a bit unsure about the comment atop ReorderBufferSetBaseSnapshot.
Obviously, the bit within the #if 0/#endif I'm going to remove before
push.

It looks like you've started editing that bit and didn't finish.

I don't understand why it says "Needs to be called before any
changes are added with ReorderBufferQueueChange"; but if you edit that
function and add an assert that the base snapshot is set, it crashes
pretty quickly in the test_decoding tests. (The supposedly bogus
comment was there before your patch -- I'm not saying your comment
addition is faulty.)

That's because REORDER_BUFFER_CHANGE_INTERNAL_COMMAND_ID changes are
queued whenever we have read that xact has modified the catalog,
regardless of base snapshot existence. Even if there are no changes yet,
we will need it for correct visibility of catalog, so I am inclined to
remove the assert and comment or rephrase the latter with 'any
*data-carrying* changes'.

* I also noticed that we're doing subtxn cleanup one by one in both
ReorderBufferAssignChild and ReorderBufferCommitChild, which means the
top-level txn is sought in the hash table over and over, which seems a
bit silly. Not this patch's problem to fix ...

There is already one-entry cache in ReorderBufferTXNByXid. We could add
'don't cache me' flag to it and use it with subxacts, or simply pull
top-level reorderbuffer out of loops.

I'm fine with the rest of your edits. One more little comment:

@@ -543,9 +546,10 @@ ReorderBufferQueueChange(ReorderBuffer *rb, TransactionId xid, XLogRecPtr lsn,
ReorderBufferTXN *txn;

txn = ReorderBufferTXNByXid(rb, xid, true, NULL, lsn, true);
+ Assert(txn->base_snapshot != NULL || txn->is_known_as_subxact);

change->lsn = lsn;
- Assert(InvalidXLogRecPtr != lsn);
+ Assert(!XLogRecPtrIsInvalid(lsn));
dlist_push_tail(&txn->changes, &change->node);
txn->nentries++;
txn->nentries_mem++;

Since we do that, probably we should replace all lsn validity checks
with XLogRectPtrIsInvalid?

--
Arseny Sher
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On 2018-Jun-26, Arseny Sher wrote:

Alvaro Herrera <alvherre@2ndquadrant.com> writes:

* I'm a bit unsure about the comment atop ReorderBufferSetBaseSnapshot.
Obviously, the bit within the #if 0/#endif I'm going to remove before
push.

It looks like you've started editing that bit and didn't finish.

Yeah, I left those lines in place as a reminder that I need to finish
editing, wondering if there's any nuance I'm missing that I need to add
to the final version.

I don't understand why it says "Needs to be called before any
changes are added with ReorderBufferQueueChange"; but if you edit that
function and add an assert that the base snapshot is set, it crashes
pretty quickly in the test_decoding tests. (The supposedly bogus
comment was there before your patch -- I'm not saying your comment
addition is faulty.)

That's because REORDER_BUFFER_CHANGE_INTERNAL_COMMAND_ID changes are
queued whenever we have read that xact has modified the catalog,
regardless of base snapshot existence. Even if there are no changes yet,
we will need it for correct visibility of catalog, so I am inclined to
remove the assert and comment or rephrase the latter with 'any
*data-carrying* changes'.

I'm struggling with this assert. I find that test_decoding passes tests
with this assertion in branch master, but fails in 9.4 - 10. Maybe I'm
running the tests wrong (no assertions in master?) but I don't see it.
It *should* fail ...

* I also noticed that we're doing subtxn cleanup one by one in both
ReorderBufferAssignChild and ReorderBufferCommitChild, which means the
top-level txn is sought in the hash table over and over, which seems a
bit silly. Not this patch's problem to fix ...

There is already one-entry cache in ReorderBufferTXNByXid.

That's useless, because we use two xids: first the parent; then the
subxact. Looking up the subxact evicts the parent from the one-entry
cache, so when we loop to process next subxact, the parent is no longer
cached :-)

We could add 'don't cache me' flag to it and use it with subxacts, or
simply pull top-level reorderbuffer out of loops.

Yeah, but that's an API change.

I'm fine with the rest of your edits. One more little comment:

@@ -543,9 +546,10 @@ ReorderBufferQueueChange(ReorderBuffer *rb, TransactionId xid, XLogRecPtr lsn,
ReorderBufferTXN *txn;

txn = ReorderBufferTXNByXid(rb, xid, true, NULL, lsn, true);
+ Assert(txn->base_snapshot != NULL || txn->is_known_as_subxact);

change->lsn = lsn;
- Assert(InvalidXLogRecPtr != lsn);
+ Assert(!XLogRecPtrIsInvalid(lsn));
dlist_push_tail(&txn->changes, &change->node);
txn->nentries++;
txn->nentries_mem++;

Since we do that, probably we should replace all lsn validity checks
with XLogRectPtrIsInvalid?

I reverted this back to how it was originally. We can change it as a
style item later in all places where it appears (branch master only),
but modifying only one in a backpatched commit seems poor practice.

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Alvaro Herrera <alvherre@2ndquadrant.com> writes:

I'm struggling with this assert. I find that test_decoding passes tests
with this assertion in branch master, but fails in 9.4 - 10. Maybe I'm
running the tests wrong (no assertions in master?) but I don't see it.
It *should* fail ...

Your v3 patch fails for me on freshest master (4d54543efa) in exactly
this assert, it looks like there is something wrong in your setup.

--
Arseny Sher
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Pushed this to 9.4 - master after some more tinkering.

It occurred to me that it might be better to have
ReorderBufferSetBaseSnapshot do the IncrRefCount instead of expecting
caller to do it. But I wouldn't backpatch that change, so I refrained.

Thanks for the patch.

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services