Re: doc: improve the restriction description of using indexes on REPLICA IDENTITY FULL table.

ed. The comments I am > > > referring to are: "Note that the limitations of index scans for > > > replica identity full only might not be a good idea in some > > > cases". Shall we move these as well atop > > > IsIndexUsableForReplicaIdentityFull()

Re: doc: improve the restriction description of using indexes on REPLICA IDENTITY FULL table.

> > Now, the comments related to limitation atop > > FindUsableIndexForReplicaIdentityFull() look slightly odd as it refers > > to limitations but those limitation were not stated. The comments I am > > referring to are: "Note that the limitations of index scans for &

Re: doc: improve the restriction description of using indexes on REPLICA IDENTITY FULL table.

m > referring to are: "Note that the limitations of index scans for > replica identity full only might not be a good idea in some > cases". Shall we move these as well atop > IsIndexUsableForReplicaIdentityFull()? Good point. Looking at neighbor

Re: doc: improve the restriction description of using indexes on REPLICA IDENTITY FULL table.

yFull(). Now, the comments related to limitation atop FindUsableIndexForReplicaIdentityFull() look slightly odd as it refers to limitations but those limitation were not stated. The comments I am referring to are: "Note that the limitations of index scans for replica identity full only might n

Re: doc: improve the restriction description of using indexes on REPLICA IDENTITY FULL table.

On Wed, Jul 19, 2023 at 5:09 PM Önder Kalacı wrote: > > Hi Masahiko, Amit, all > >> I've updated the patch. > > > I think the flow is much nicer now compared to the HEAD. I really don't have > any > comments regarding the accuracy of the code changes, all looks good to me. > Overall, I cannot

Re: doc: improve the restriction description of using indexes on REPLICA IDENTITY FULL table.

Hi Masahiko, Amit, all I've updated the patch. > I think the flow is much nicer now compared to the HEAD. I really don't have any comments regarding the accuracy of the code changes, all looks good to me. Overall, I cannot see any behavioral changes as you already alluded to. Maybe few minor

Re: doc: improve the restriction description of using indexes on REPLICA IDENTITY FULL table.

On Tue, Jul 18, 2023 at 12:10 PM Masahiko Sawada wrote: > > BTW, IsIndexOnlyExpression() is not necessary but the current code > still works fine. So do we need to backpatch it to PG16? I'm thinking > we can apply it to only HEAD. > Either way is fine but I think if we backpatch it then the code

Re: doc: improve the restriction description of using indexes on REPLICA IDENTITY FULL table.

* relation column. These limitations help to keep the index scan similar - * to PK/RI index scans. + * the relation. I moved the second sentence to IsIndexUsableForReplicaIdentityFull() because this function is now responsible for checking if the given index is usable for REPLICA IDENTITY FUL

Re: doc: improve the restriction description of using indexes on REPLICA IDENTITY FULL table.

Hi, > I've attached a draft patch for this idea. I think it needs a rebase after edca3424342da323499a1998d18a888283e52ac7. Also, as discussed in [1], I think one improvement we had was to keep IsIndexUsableForReplicaIdentityFull() in a way that it is easier to read & better documented in the

Re: doc: improve the restriction description of using indexes on REPLICA IDENTITY FULL table.

On Thu, Jul 13, 2023 at 10:55 AM Masahiko Sawada wrote: > > On Wed, Jul 12, 2023 at 11:15 PM Masahiko Sawada > wrote: > > > > > I think this is a valid concern. Can't we move all the checks > > > (including the remote attrs check) inside > > > IsIndexUsableForReplicaIdentityFull() and then call

Re: doc: improve the restriction description of using indexes on REPLICA IDENTITY FULL table.

On Wed, Jul 12, 2023 at 11:15 PM Masahiko Sawada wrote: > > On Wed, Jul 12, 2023 at 7:08 PM Amit Kapila wrote: > > > > On Wed, Jul 12, 2023 at 12:31 PM Masahiko Sawada > > wrote: > > > > > > On Tue, Jul 11, 2023 at 5:31 PM Peter Smith wrote: > > > > > > > > > > I don't think we have concluded

Re: doc: improve the restriction description of using indexes on REPLICA IDENTITY FULL table.

On Thu, Jul 13, 2023 at 12:22 PM Masahiko Sawada wrote: > > On Thu, Jul 13, 2023 at 11:12 AM Peter Smith wrote: > > > > On Thu, Jul 13, 2023 at 11:28 AM Masahiko Sawada > > wrote: > > > > > > On Thu, Jul 13, 2023 at 8:03 AM Peter Smith wrote: > > > > > > > > On Wed, Jul 12, 2023 at 5:01 PM

Re: doc: improve the restriction description of using indexes on REPLICA IDENTITY FULL table.

On Thu, Jul 13, 2023 at 11:12 AM Peter Smith wrote: > > On Thu, Jul 13, 2023 at 11:28 AM Masahiko Sawada > wrote: > > > > On Thu, Jul 13, 2023 at 8:03 AM Peter Smith wrote: > > > > > > On Wed, Jul 12, 2023 at 5:01 PM Masahiko Sawada > > > wrote: > > > > > > > > On Tue, Jul 11, 2023 at 5:31 

Re: doc: improve the restriction description of using indexes on REPLICA IDENTITY FULL table.

On Thu, Jul 13, 2023 at 11:28 AM Masahiko Sawada wrote: > > On Thu, Jul 13, 2023 at 8:03 AM Peter Smith wrote: > > > > On Wed, Jul 12, 2023 at 5:01 PM Masahiko Sawada > > wrote: > > > > > > On Tue, Jul 11, 2023 at 5:31 PM Peter Smith wrote: > > > > ... > > > > I checked v5-0001 and noticed

Re: doc: improve the restriction description of using indexes on REPLICA IDENTITY FULL table.

On Thu, Jul 13, 2023 at 8:03 AM Peter Smith wrote: > > On Wed, Jul 12, 2023 at 5:01 PM Masahiko Sawada wrote: > > > > On Tue, Jul 11, 2023 at 5:31 PM Peter Smith wrote: > > > > > > Here are my comments for v4. > > > > > > == > > > > > > Docs/Comments: > > > > > > > > > > > Agreed. I've

Re: doc: improve the restriction description of using indexes on REPLICA IDENTITY FULL table.

On Wed, Jul 12, 2023 at 5:01 PM Masahiko Sawada wrote: > > On Tue, Jul 11, 2023 at 5:31 PM Peter Smith wrote: > > > > Here are my comments for v4. > > > > == > > > > Docs/Comments: > > > > > > Agreed. I've attached the updated patch. I'll push it barring any objections. > > > I

Re: doc: improve the restriction description of using indexes on REPLICA IDENTITY FULL table.

On Wed, Jul 12, 2023 at 7:08 PM Amit Kapila wrote: > > On Wed, Jul 12, 2023 at 12:31 PM Masahiko Sawada > wrote: > > > > On Tue, Jul 11, 2023 at 5:31 PM Peter Smith wrote: > > > > > > > I don't think we have concluded any action for it. I agree that > > IsIndexOnlyOnExpression() is redundant.

Re: doc: improve the restriction description of using indexes on REPLICA IDENTITY FULL table.

Hi Amit, all Amit Kapila , 12 Tem 2023 Çar, 13:09 tarihinde şunu yazdı: > On Wed, Jul 12, 2023 at 12:31 PM Masahiko Sawada > wrote: > > > > On Tue, Jul 11, 2023 at 5:31 PM Peter Smith > wrote: > > > > > > > I don't think we have concluded any action for it. I agree that > >

Re: doc: improve the restriction description of using indexes on REPLICA IDENTITY FULL table.

On Wed, Jul 12, 2023 at 12:31 PM Masahiko Sawada wrote: > > On Tue, Jul 11, 2023 at 5:31 PM Peter Smith wrote: > > > > I don't think we have concluded any action for it. I agree that > IsIndexOnlyOnExpression() is redundant. We don't need to check *all* > index fields actually. I've attached a

Re: doc: improve the restriction description of using indexes on REPLICA IDENTITY FULL table.

On Tue, Jul 11, 2023 at 5:31 PM Peter Smith wrote: > > Here are my comments for v4. > > == > > Docs/Comments: > > All the docs and updated comments LTGM, except I felt one sentence > might be written differently to avoid nested parentheses. > > BEFORE > ...

Re: doc: improve the restriction description of using indexes on REPLICA IDENTITY FULL table.

Here are my comments for v4. == Docs/Comments: All the docs and updated comments LTGM, except I felt one sentence might be written differently to avoid nested parentheses. BEFORE ...used for REPLICA IDENTITY FULL table (see FindUsableIndexForReplicaIdentityFull() for details). AFTER

Re: doc: improve the restriction description of using indexes on REPLICA IDENTITY FULL table.

On Tue, Jul 11, 2023 at 1:05 PM Amit Kapila wrote: > > On Tue, Jul 11, 2023 at 4:54 AM Peter Smith wrote: > > > > On Mon, Jul 10, 2023 at 2:21 PM Amit Kapila wrote: > > > > > > On Mon, Jul 10, 2023 at 7:55 AM Peter Smith wrote: > > > > > > > > On Sat, Jul 8, 2023 at 1:49 PM Amit Kapila > > >

Re: doc: improve the restriction description of using indexes on REPLICA IDENTITY FULL table.

On Tue, Jul 11, 2023 at 4:54 AM Peter Smith wrote: > > On Mon, Jul 10, 2023 at 2:21 PM Amit Kapila wrote: > > > > On Mon, Jul 10, 2023 at 7:55 AM Peter Smith wrote: > > > > > > On Sat, Jul 8, 2023 at 1:49 PM Amit Kapila > > > wrote: > > > > > > > > On Fri, Jul 7, 2023 at 1:36 PM Masahiko

Re: doc: improve the restriction description of using indexes on REPLICA IDENTITY FULL table.

On Mon, Jul 10, 2023 at 2:21 PM Amit Kapila wrote: > > On Mon, Jul 10, 2023 at 7:55 AM Peter Smith wrote: > > > > On Sat, Jul 8, 2023 at 1:49 PM Amit Kapila wrote: > > > > > > On Fri, Jul 7, 2023 at 1:36 PM Masahiko Sawada > > > wrote: > > > > > > > > I prefer the first suggestion. I've

Re: doc: improve the restriction description of using indexes on REPLICA IDENTITY FULL table.

On Mon, Jul 10, 2023 at 7:55 AM Peter Smith wrote: > > On Sat, Jul 8, 2023 at 1:49 PM Amit Kapila wrote: > > > > On Fri, Jul 7, 2023 at 1:36 PM Masahiko Sawada > > wrote: > > > > > > I prefer the first suggestion. I've attached the updated patch. > > > > > > > This looks mostly good to me but

Re: doc: improve the restriction description of using indexes on REPLICA IDENTITY FULL table.

On Sat, Jul 8, 2023 at 1:49 PM Amit Kapila wrote: > > On Fri, Jul 7, 2023 at 1:36 PM Masahiko Sawada wrote: > > > > I prefer the first suggestion. I've attached the updated patch. > > > > This looks mostly good to me but I think it would be better if we can > also add the information that the

Re: doc: improve the restriction description of using indexes on REPLICA IDENTITY FULL table.

On Fri, Jul 7, 2023 at 1:36 PM Masahiko Sawada wrote: > > I prefer the first suggestion. I've attached the updated patch. > This looks mostly good to me but I think it would be better if we can also add the information that the leftmost index column must be a non-expression. So, how about:

Re: doc: improve the restriction description of using indexes on REPLICA IDENTITY FULL table.

On Fri, Jul 7, 2023 at 10:55 AM Peter Smith wrote: > > Hi, Here are my review comments for patch v2. > > == > 1. doc/src/sgml/logical-replication.sgml > > Candidate indexes must be btree, non-partial, and have at least one > column reference to the published table column at the leftmost

Re: doc: improve the restriction description of using indexes on REPLICA IDENTITY FULL table.

Hi, Here are my review comments for patch v2. == 1. doc/src/sgml/logical-replication.sgml Candidate indexes must be btree, non-partial, and have at least one column reference to the published table column at the leftmost column (i.e. cannot consist of only expressions). ~ There is only one

Re: doc: improve the restriction description of using indexes on REPLICA IDENTITY FULL table.

ht this is the reason why we have separate IsIndexOnlyOnExpression() ( and IsIndexUsableForReplicaIdentityFull()). But this assertion doesn't check if the leftmost index column exists on the remote relation. What are we doing this check for? If it's not necessary, we can remove this assertion and mer

Re: doc: improve the restriction description of using indexes on REPLICA IDENTITY FULL table.

ction seemed redundant to me, otherwise, there can be some indexes that are firstly considered "useable" but then fail the subsequent leftmost check. It does not seem right. > > > doc/src/sgml/logical-replication.sgml > > > > > > 1. > > >

Re: doc: improve the restriction description of using indexes on REPLICA IDENTITY FULL table.

On Wed, Jul 5, 2023 at 12:02 PM Masahiko Sawada wrote: > > On Wed, Jul 5, 2023 at 2:46 PM Amit Kapila wrote: > > > > On Wed, Jul 5, 2023 at 9:01 AM Peter Smith wrote: > > > > > > Hi. Here are some review comments for this patch. > > > > > > +1 for the patch idea. > > > > > > -- > > > > > >

Re: doc: improve the restriction description of using indexes on REPLICA IDENTITY FULL table.

check if the leftmost index column is a non-expression. > > doc/src/sgml/logical-replication.sgml > > > > 1. > > the key. When replica identity FULL is specified, > > indexes can be used on the subscriber side for searching the rows. > > Candidat

Re: doc: improve the restriction description of using indexes on REPLICA IDENTITY FULL table.

we don't even call FindUsableIndexForReplicaIdentityFull() but build_replindex_scan_key() would still be called for such index. So, I am not sure your proposed wording is an improvement. > -- > doc/src/sgml/logical-replication.sgml > > 1. > the key. When replica identity FU

Re: doc: improve the restriction description of using indexes on REPLICA IDENTITY FULL table.

ic routine. It expects the 'idxrel' to be an index deemed "usable" by the function FindUsableIndexForReplicaIdentityFull(). -- doc/src/sgml/logical-replication.sgml 1. the key. When replica identity FULL is specified, indexes can be used on the subscriber side for searching

Re: doc: improve the restriction description of using indexes on REPLICA IDENTITY FULL table.

On Mon, Jul 3, 2023 at 7:45 AM Masahiko Sawada wrote: > > Commit 89e46da5e5 allowed us to use indexes for searching on REPLICA > IDENTITY FULL tables. The documentation explains: > > When replica identity full is specified, > indexes can be used on the subscriber side for

doc: improve the restriction description of using indexes on REPLICA IDENTITY FULL table.

Hi, Commit 89e46da5e5 allowed us to use indexes for searching on REPLICA IDENTITY FULL tables. The documentation explains: When replica identity full is specified, indexes can be used on the subscriber side for searching the rows. Candidate indexes must be btree, non-partial, and have at least

Re: Dropped and generated columns might cause wrong data on subs when REPLICA IDENTITY FULL

On Wed, Mar 22, 2023 at 1:39 PM shiy.f...@fujitsu.com wrote: > > On Wed, Mar 22, 2023 2:53 PM Önder Kalacı wrote: > > > > We don't really need to, if you check the first patch, we don't have DROP > > for generated case. I mostly > > wanted to make the test a little more interesting, but it also

RE: Dropped and generated columns might cause wrong data on subs when REPLICA IDENTITY FULL

On Wed, Mar 22, 2023 2:53 PM Önder Kalacı wrote: > > We don't really need to, if you check the first patch, we don't have DROP for > generated case. I mostly > wanted to make the test a little more interesting, but it also seems to be a > little confusing. > > Now attaching v2 where we do not

Re: Dropped and generated columns might cause wrong data on subs when REPLICA IDENTITY FULL

Hi Shi Yu, > > Is there any reasons why we drop column here? Dropped column case has been > tested on table dropped_cols. The generated column problem can be detected > without dropping columns on my machine. > We don't really need to, if you check the first patch, we don't have DROP for

RE: Dropped and generated columns might cause wrong data on subs when REPLICA IDENTITY FULL

On Tuesday, March 21, 2023 8:03 PM Önder Kalacı wrote: > > Attached patches again. > Thanks for updating the patch. @@ -408,15 +412,18 @@ $node_subscriber->wait_for_subscription_sync; $node_publisher->safe_psql( 'postgres', qq( ALTER TABLE dropped_cols DROP COLUMN

Re: Dropped and generated columns might cause wrong data on subs when REPLICA IDENTITY FULL

etected. > > Ow, what a mistake. Now changed (and ensured that without the patch the test fails). > 2. > # The bug was that when the REPLICA IDENTITY FULL is used with dropped > columns, > # we fail to apply updates and deletes > > Maybe we should mention generated column

RE: Dropped and generated columns might cause wrong data on subs when REPLICA IDENTITY FULL

est generated columns, so we don't need to drop columns. Otherwise the generated column problem can't be detected. 2. # The bug was that when the REPLICA IDENTITY FULL is used with dropped columns, # we fail to apply updates and deletes Maybe we should mention generated columns in comment of the tes

Re: Dropped and generated columns might cause wrong data on subs when REPLICA IDENTITY FULL

Hi Amit, Shi Yu Now attaching the similar patches for generated columns. Thanks, Onder KALACI Amit Kapila , 21 Mar 2023 Sal, 09:07 tarihinde şunu yazdı: > On Mon, Mar 20, 2023 at 6:28 PM Amit Kapila > wrote: > > > > On Mon, Mar 20, 2023 at 2:58 PM Önder Kalacı > wrote: > > > > > > > > > I

Re: Dropped and generated columns might cause wrong data on subs when REPLICA IDENTITY FULL

On Mon, Mar 20, 2023 at 6:28 PM Amit Kapila wrote: > > On Mon, Mar 20, 2023 at 2:58 PM Önder Kalacı wrote: > > > > > > I applied all patches for all the versions, and re-run the subscription > > tests, > > all looks good to me. > > > > LGTM. I'll push this tomorrow unless there are more

Re: Dropped and generated columns might cause wrong data on subs when REPLICA IDENTITY FULL

On Mon, Mar 20, 2023 at 2:58 PM Önder Kalacı wrote: > > > I applied all patches for all the versions, and re-run the subscription tests, > all looks good to me. > LGTM. I'll push this tomorrow unless there are more comments. -- With Regards, Amit Kapila.

Re: Dropped and generated columns might cause wrong data on subs when REPLICA IDENTITY FULL

Hi Shi Yu, all Thanks for updating the patches. It seems you forgot to attach the patches > of > dropped columns for HEAD and pg15, I think they are the same as v2. > > Yes, it seems I forgot. And, yes they were the same as v2. > On HEAD, we can re-use clusters in other test cases, which can

RE: Dropped and generated columns might cause wrong data on subs when REPLICA IDENTITY FULL

On Fri, Mar 17, 2023 11:29 PM Önder Kalacı wrote: > > Thanks for sharing. Fixed > > > This time I was able to run all the tests with all the patches applied. > > Again, the generated column fix also has some minor differences > per version. So, overall we have 6 patches with very minor >

Re: Dropped and generated columns might cause wrong data on subs when REPLICA IDENTITY FULL

Hi Shi Yu, Thanks for the review, really appreciate it! > I couldn't apply > v2-0001-Ignore-dropped-columns-HEAD-REL_15-REL_14-REL_13.patch > cleanly in v13 and v14. It looks the patch needs some changes in these > versions. > > > ``` > Checking patch src/backend/executor/execReplication.c...

RE: Dropped and generated columns might cause wrong data on subs when REPLICA IDENTITY FULL

On Friday, March 17, 2023 3:38 PM Önder Kalacı wrote: > > Hi Amit, all > > You can first submit the fix for dropped columns with patches till > v10. Once that is committed, then you can send the patches for > generated columns. > > Alright, attaching 2 patches for dropped columns, the names

Re: Dropped and generated columns might cause wrong data on subs when REPLICA IDENTITY FULL

Hi Amit, all > You can first submit the fix for dropped columns with patches till > v10. Once that is committed, then you can send the patches for > generated columns. > > Alright, attaching 2 patches for dropped columns, the names of the files shows which versions the patch can be applied to:

Re: Dropped and generated columns might cause wrong data on subs when REPLICA IDENTITY FULL

On Fri, Mar 17, 2023 at 5:41 AM Tom Lane wrote: > > Amit Kapila writes: > > On Thu, Mar 16, 2023 at 9:33 PM Önder Kalacı wrote: > >>> and backpatch the fix for dropped column to PG10. > > > You can first submit the fix for dropped columns with patches till > > v10. Once that is committed, then

Re: Dropped and generated columns might cause wrong data on subs when REPLICA IDENTITY FULL

Amit Kapila writes: > On Thu, Mar 16, 2023 at 9:33 PM Önder Kalacı wrote: >>> and backpatch the fix for dropped column to PG10. > You can first submit the fix for dropped columns with patches till > v10. Once that is committed, then you can send the patches for > generated columns. Don't worry

Re: Dropped and generated columns might cause wrong data on subs when REPLICA IDENTITY FULL

On Thu, Mar 16, 2023 at 9:33 PM Önder Kalacı wrote: > > Hi Amit, Shi Yu, > > > Generated column is introduced in PG12, and I reproduced generated column > > problem > in PG12~PG15. > > For dropped column problem, I reproduced it in PG10~PG15. (Logical > > replication > was introduced in PG10) >

Re: Dropped and generated columns might cause wrong data on subs when REPLICA IDENTITY FULL

publisher doesn't send those."? After your change, att = > > TupleDescAttr(slot1->tts_tupleDescriptor, attrnum); is done twice in > > the same function. > > > > In test cases, let's change the comment to: "The bug was that when the > > REPLICA IDENT

RE: Dropped and generated columns might cause wrong data on subs when REPLICA IDENTITY FULL

After your change, att = > TupleDescAttr(slot1->tts_tupleDescriptor, attrnum); is done twice in > the same function. > > In test cases, let's change the comment to: "The bug was that when the > REPLICA IDENTITY FULL is used with dropped or generated columns, we > fail

Re: Dropped and generated columns might cause wrong data on subs when REPLICA IDENTITY FULL

e in the same function. In test cases, let's change the comment to: "The bug was that when the REPLICA IDENTITY FULL is used with dropped or generated columns, we fail to apply updates and deletes.". Also, I think we don't need to provide the email link as anyway commit message will have a

Re: Dropped and generated columns might cause wrong data on subs when REPLICA IDENTITY FULL

t;tts_tupleDescriptor, attrnum); > + > > I think we can use "att" instead of a new variable. They have the same > value. > ah, of course :) > > 2. > +# The bug was that when when the REPLICA IDENTITY FULL is used with > dropped > > There is an extra "when&qu

RE: Dropped and generated columns might cause wrong data on subs when REPLICA IDENTITY FULL

when when the REPLICA IDENTITY FULL is used with dropped There is an extra "when". Regards, Shi Yu

Dropped and generated columns might cause wrong data on subs when REPLICA IDENTITY FULL

lumns, which indeed has the same problem. Here are the steps to repro the problem with dropped columns: - pub CREATE TABLE test (drop_1 jsonb, x int, drop_2 numeric, y text, drop_3 timestamptz); ALTER TABLE test REPLICA IDENTITY FULL; INSERT INTO test SELECT NULL, i, i, (i)::text, now() FROM generate_

Re: Problem with logical replication (crash with REPLICA IDENTITY FULL and cascading replication)

or a in data data1; do ./tmp_install/usr/local/pgsql/bin/postmaster -D $a& done for a in "CREATE TABLE pgbench_accounts(a int primary key, b int)" "ALTER TABLE pgbench_accounts REPLICA IDENTITY FULL" "CREATE PUBLICATION mypub FOR TABLE pgbench_accounts"; d

Re: REPLICA IDENTITY FULL

On Fri, Jun 23, 2017 at 03:45:48PM -0400, Peter Eisentraut wrote: > On 6/23/17 13:14, Alvaro Herrera wrote: > > Andres Freund wrote: > >> On 2017-06-23 13:05:21 -0400, Alvaro Herrera wrote: > >>> Tom Lane wrote: > Peter Eisentraut writes: > > Any