Message-ID: <9d7635f8-2402-e038-64da-cbc3732ce8cf@enterprisedb.com>
Date: Fri, 25 Mar 2022 11:26:39 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.7.0
Subject: Re: logical decoding and replication of sequences
Content-Language: en-US
To: Amit Kapila <amit.kapila16@gmail.com>
Cc: Petr Jelinek <petr.jelinek@enterprisedb.com>,
 Peter Eisentraut <peter.eisentraut@enterprisedb.com>,
 PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
References: <d045f3c2-6cfb-06d3-5540-e63c320df8bc@enterprisedb.com>
 <f242d058-be79-6a59-5497-0bb256cafcc7@enterprisedb.com>
 <e890448d-00b5-18d0-e894-85d17660d484@enterprisedb.com>
 <9defb937-20a9-1d9f-6972-f1c4b4da4f73@enterprisedb.com>
 <8e766e0f-ed3e-fd68-4b0f-10dd2db60c88@enterprisedb.com>
 <c38fd4df-f71a-cf47-88cd-5f486b5bca92@enterprisedb.com>
 <f64b0ecd-0723-c70d-8ec5-2b5c0e476a50@enterprisedb.com>
 <6b21233e-c880-e4ad-5df2-7832f769296c@enterprisedb.com>
 <9da788c7-01eb-f11d-aa08-c5f61fe9f33a@enterprisedb.com>
 <CAA4eK1Jro8hGX_hzXCznBr++suoDM7avEhFtiphPeDLPBOc=Ow@mail.gmail.com>
 <456A528F-8F19-41C7-9857-927024D32396@enterprisedb.com>
 <CAA4eK1Jn-DttQ=4Pdh9YCe1w+zGbgC+0uR0sfg2RtkjiAPmB9g@mail.gmail.com>
 <BDDB28EE-84D3-4BA8-A5BD-DFF56CE584BC@enterprisedb.com>
 <aeb2ba8d-e6f4-5486-cc4c-0d4982c291cb@enterprisedb.com>
 <74729cc5-960b-9295-912a-a7c37356a0c4@enterprisedb.com>
 <CAA4eK1JxDNKGBSNTyN-Xj2JRjzFo+ziSqJbjH==vuO0YF_CQrg@mail.gmail.com>
From: Tomas Vondra <tomas.vondra@enterprisedb.com>
In-Reply-To: 
 <CAA4eK1JxDNKGBSNTyN-Xj2JRjzFo+ziSqJbjH==vuO0YF_CQrg@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/9d7635f8-2402-e038-64da-cbc3732ce8cf%40enterprisedb.com>
Precedence: bulk


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