Re: [HACKERS] logical decoding of two-phase transactions

>> PFA, latest patchset, which completely removes the earlier
>> LogicalLock/LogicalUnLock implementation using groupDecode stuff and
>> uses the newly suggested approach of checking the currently decoded
>> XID for abort in systable_* API functions. Much simpler to code and
>> easier to test as well.
>
> So, leaving the fact that it might not actually be correct aside ;), you
> seem to be ok with the approach?
>

;-)

Yes, I do like the approach. Do you think there are other locations
other than systable_* APIs which might need such checks?

>> There's an additional test case in
>> 0005-Additional-test-case-to-demonstrate-decoding-rollbac.patch which
>> uses a sleep in the "change" plugin API to allow a concurrent rollback
>> on the 2PC being currently decoded. Andres generally doesn't like this
>> approach :-), but there are no timing/interlocking issues now, and the
>> sleep just helps us do a concurrent rollback, so it might be ok now,
>> all things considered. Anyways, it's an additional patch for now.
>
> Yea, I still don't think it's ok. The tests won't be reliable. There's
> ways to make this reliable, e.g. by forcing a lock to be acquired that's
> externally held or such. Might even be doable just with a weird custom
> datatype.
>

Ok, I will look at ways to do away with the sleep.

>> diff --git a/src/backend/access/index/genam.c b/src/backend/access/index/genam.c
>> index 9d08775687..67c5810bf7 100644
>> --- a/src/backend/access/index/genam.c
>> +++ b/src/backend/access/index/genam.c
>> @@ -423,6 +423,16 @@ systable_getnext(SysScanDesc sysscan)
>> else
>> htup = heap_getnext(sysscan->scan, ForwardScanDirection);
>>
>> + /*
>> + * If CheckXidAlive is valid, then we check if it aborted. If it did, we
>> + * error out
>> + */
>> + if (TransactionIdIsValid(CheckXidAlive) &&
>> + TransactionIdDidAbort(CheckXidAlive))
>> + ereport(ERROR,
>> + (errcode(ERRCODE_TRANSACTION_ROLLBACK),
>> + errmsg("transaction aborted during system catalog scan")));
>> +
>> return htup;
>> }
>
> Don't we have to check TransactionIdIsInProgress() first? C.f. header
> comments in tqual.c. Note this is also not guaranteed to be correct
> after a crash (where no clog entry will exist for an aborted xact), but
> we probably shouldn't get here in that case - but better be safe.
>
> I suspect it'd be better reformulated as
> TransactionIdIsValid(CheckXidAlive) &&
> !TransactionIdIsInProgress(CheckXidAlive) &&
> !TransactionIdDidCommit(CheckXidAlive)
>
> What do you think?
>

tqual.c does seem to mention this for a non-MVCC snapshot, so might as
well do it this ways. The caching of fetched XID should not make these
checks too expensive anyways.

>
> I think it'd also be good to add assertions to codepaths not going
> through systable_* asserting that
> !TransactionIdIsValid(CheckXidAlive). Alternatively we could add an
> if (unlikely(TransactionIdIsValid(CheckXidAlive)) && ...)
> branch to those too.
>

I was wondering if anything else would be needed for user-defined
catalog tables..

>
>
>> From 80fc576bda483798919653991bef6dc198625d90 Mon Sep 17 00:00:00 2001
>> From: Nikhil Sontakke <nikhils(at)2ndQuadrant(dot)com>
>> Date: Wed, 13 Jun 2018 16:31:15 +0530
>> Subject: [PATCH 4/5] Teach test_decoding plugin to work with 2PC
>>
>> Includes a new option "enable_twophase". Depending on this options
>> value, PREPARE TRANSACTION will either be decoded or treated as
>> a single phase commit later.
>
> FWIW, I don't think I'm ok with doing this on a per-plugin-option basis.
> I think this is something that should be known to the outside of the
> plugin. More similar to how binary / non-binary support works. Should
> also be able to inquire the output plugin whether it's supported (cf
> previous similarity).
>

Hmm, lemme see if we can do it outside of the plugin. But note that a
plugin might want to decode some 2PC at prepare time and another are
"commit prepared" time.

We also need to take care to not break logical replication if the
other node is running non-2PC enabled code. We tried to optimize the
COMMIT/ABORT handling by adding sub flags to the existing protocol. I
will test that as well.

Regards,
Nikhils
--
Nikhil Sontakke http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services