Re: [HACKERS] logical decoding of two-phase transactions - Mailing list pgsql-hackers

From Petr Jelinek
Subject Re: [HACKERS] logical decoding of two-phase transactions
Date
Msg-id 2e1cdeb7-6940-8558-910a-9673b507bacb@2ndquadrant.com
Whole thread Raw
In response to Re: [HACKERS] logical decoding of two-phase transactions  (Arseny Sher <a.sher@postgrespro.ru>)
List pgsql-hackers
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


pgsql-hackers by date:

Previous
From: Nick B
Date:
Subject: pg_basebackup, walreceiver and wal_sender_timeout
Next
From: Heikki Linnakangas
Date:
Subject: Re: Speeding up text_position_next with multibyte encodings