On 14/01/2019 23:16, Arseny Sher wrote:
>
> Nikhil Sontakke <nikhils@2ndquadrant.com> writes:
>
>> I'd like to believe that the latest patch set tries to address some
>> (if not all) of your concerns. Can you please take a look and let me
>> know?
>
> Hi, sure.
>
> General things:
>
> - Earlier I said that there is no point of sending COMMIT PREPARED if
> decoding snapshot became consistent after PREPARE, i.e. PREPARE hadn't
> been sent. I realized since then that such use cases actually exist:
> prepare might be copied to the replica by e.g. basebackup or something
> else earlier.
Basebackup does not copy slots though and slot should not reach
consistency until all prepared transactions are committed no?
>
> - BTW, ReorderBufferProcessXid at PREPARE should be always called
> anyway, because otherwise if xact is empty, we will not prepare it
> (and call cb), even if the output plugin asked us not to filter it
> out. However, we will call commit_prepared cb, which is inconsistent.
>
> - I find it weird that in DecodePrepare and in DecodeCommit you always
> ask the plugin whether to filter an xact, given that sometimes you
> know beforehand that you are not going to replay it: it might have
> already been replayed, might have wrong dbid, origin, etc. One
> consequence of this: imagine that notorious xact with PREPARE before
> point where snapshot became consistent and COMMIT PREPARED after that
> point. Even if filter_cb says 'I want 2PC on this xact', with current
> code it won't be replayed on PREPARE and rbxid will be destroyed with
> ReorderBufferForget. Now this xact is lost.
Yeah this is wrong.
>
> Second patch:
>
> + /* filter_prepare is optional, but requires two-phase decoding */
> + if ((ctx->callbacks.filter_prepare_cb != NULL) && (!opt->enable_twophase))
> + ereport(ERROR,
> + (errmsg("Output plugin does not support two-phase decoding, but "
> + "registered filter_prepared callback.")));
>
> I actually think that enable_twophase output plugin option is
> redundant. If plugin author wants 2PC, he just provides
> filter_prepare_cb callback and potentially others.
+1
> I also don't see much
> value in checking that exactly 0 or 3 callbacks were registred.
>
I think that check makes sense, if you support 2pc you need to register
all callbacks.
>
> Nitpicking:
>
> First patch: I still don't think that these flags need a bitmask.
Since we are discussing this, I personally prefer the bitmask here.
--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services