On Mon, Nov 27, 2023 at 11:45 PM Tomas Vondra
<tomas.vondra@enterprisedb.com> wrote:
>
> I spent a bit of time looking at the proposed change, and unfortunately
> logging just the boolean flag does not work. A good example is this bit
> from a TAP test added by the patch for built-in replication (which was
> not included with the WIP patch):
>
> BEGIN;
> ALTER SEQUENCE s RESTART WITH 1000;
> SAVEPOINT sp1;
> INSERT INTO seq_test SELECT nextval('s') FROM generate_series(1,100);
> ROLLBACK TO sp1;
> COMMIT;
>
> This is expected to produce:
>
> 1131|0|t
>
> but produces
>
> 1000|0|f
>
> instead. The reason is very simple - as implemented, the patch simply
> checks if the relfilenode is from the same top-level transaction, which
> it is, and sets the flag to "true". So we know the sequence changes need
> to be queued and replayed as part of this transaction.
>
> But then during decoding, we still queue the changes into the subxact,
> which then aborts, and the changes are discarded. That is not how it's
> supposed to work, because the new relfilenode is still valid, someone
> might do nextval() and commit. And the nextval() may not get WAL-logged,
> so we'd lose this.
>
> What I guess we might do is log not just a boolean flag, but the XID of
> the subtransaction that created the relfilenode. And then during
> decoding we'd queue the changes into this subtransaction ...
>
> 0006 in the attached patch series does this, and it seems to fix the TAP
> test failure. I left it at the end, to make it easier to run tests
> without the patch applied.
>
Offhand, I don't have any better idea than what you have suggested for
the problem but this needs some thoughts including the questions asked
by you. I'll spend some time on it and respond back.
--
With Regards,
Amit Kapila.