Re: logical decoding and replication of sequences

On 3/25/22 05:01, Amit Kapila wrote:
> On Fri, Mar 25, 2022 at 3:29 AM Tomas Vondra
> <tomas.vondra@enterprisedb.com> wrote:
>>
>> Pushed.
>>
> 
> Some of the comments given by me [1] don't seem to be addressed or
> responded to. Let me try to say again for the ease of discussion:
> 

D'oh! I got distracted by Petr's response to that message, and missed
this part ...

> * Don't we need some syncing mechanism between apply worker and
> sequence sync worker so that apply worker skips the sequence changes
> till the sync worker is finished, otherwise, there is a risk of one
> overriding the values of the other? See how we take care of this for a
> table in should_apply_changes_for_rel() and its callers. If we don't
> do this for sequences for some reason then probably a comment
> somewhere is required.
> 

How would that happen? If we're effectively setting the sequence as a
side effect of inserting the data, then why should we even replicate the
sequence? We'll have the problem later too, no?

> * Don't we need explicit privilege checking before applying sequence
> data as we do in commit a2ab9c06ea15fbcb2bfde570986a06b37f52bcca for
> tables?
> 

So essentially something like TargetPrivilegesCheck in the worker? I
think you're probably right we need something like that.

> Few new comments:
> =================
> 1. A simple test like the below crashes for me:
> postgres=# create sequence s1;
> CREATE SEQUENCE
> postgres=# create sequence s2;
> CREATE SEQUENCE
> postgres=# create publication pub1 for sequence s1, s2;
> server closed the connection unexpectedly
> This probably means the server terminated abnormally
> before or while processing the request.
> The connection to the server was lost. Attempting reset: Failed.
> 

Yeah, preprocess_pubobj_list seems to be a few bricks shy. I have a fix,
will push shortly.

> 2. In apply_handle_sequence() do we need AccessExclusiveLock for
> non-transactional case?
> 

Good catch. This lock was inherited from ResetSequence, but now that the
transactional case works differently, we probably don't need it.

> 3. In apply_handle_sequence(), I think for transactional case, we need
> to skip the operation, if the skip lsn is set. See how we skip in
> apply_handle_insert() and similar functions.
> 

Right.


Thanks for these reports!

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company