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

2023-07-25 Thread Masahiko Sawada
On Mon, Jul 24, 2023 at 12:05 PM Amit Kapila wrote: > > On Mon, Jul 24, 2023 at 6:39 AM Masahiko Sawada wrote: > > > > On Sat, Jul 22, 2023 at 7:32 PM Amit Kapila wrote: > > > > > > > > > You have moved most of the comments related to the restriction of > > > which index can be picked atop

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

2023-07-23 Thread Amit Kapila
On Mon, Jul 24, 2023 at 6:39 AM Masahiko Sawada wrote: > > On Sat, Jul 22, 2023 at 7:32 PM Amit Kapila wrote: > > > > > > You have moved most of the comments related to the restriction of > > which index can be picked atop IsIndexUsableForReplicaIdentityFull(). > > Now, the comments related to

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

2023-07-23 Thread Masahiko Sawada
On Sat, Jul 22, 2023 at 7:32 PM Amit Kapila wrote: > > On Fri, Jul 21, 2023 at 6:55 AM Masahiko Sawada wrote: > > > > I've attached the updated patch. I'll push it early next week, barring > > any objections. > > > > You have moved most of the comments related to the restriction of > which index

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

2023-07-22 Thread Amit Kapila
On Fri, Jul 21, 2023 at 6:55 AM Masahiko Sawada wrote: > > I've attached the updated patch. I'll push it early next week, barring > any objections. > You have moved most of the comments related to the restriction of which index can be picked atop IsIndexUsableForReplicaIdentityFull(). Now, the

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

2023-07-20 Thread Masahiko Sawada
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.

2023-07-19 Thread Önder Kalacı
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.

2023-07-18 Thread Amit Kapila
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.

2023-07-18 Thread Masahiko Sawada
On Sat, Jul 15, 2023 at 2:11 PM Amit Kapila wrote: > > 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

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

2023-07-17 Thread Önder Kalacı
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.

2023-07-14 Thread Amit Kapila
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.

2023-07-12 Thread Masahiko Sawada
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.

2023-07-12 Thread Peter Smith
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.

2023-07-12 Thread Masahiko Sawada
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.

2023-07-12 Thread Peter Smith
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.

2023-07-12 Thread Masahiko Sawada
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.

2023-07-12 Thread Peter Smith
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.

2023-07-12 Thread Masahiko Sawada
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.

2023-07-12 Thread Önder Kalacı
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.

2023-07-12 Thread Amit Kapila
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.

2023-07-12 Thread Masahiko Sawada
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 > ...used for REPLICA IDENTITY FULL table

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

2023-07-11 Thread Peter Smith
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.

2023-07-10 Thread Masahiko Sawada
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.

2023-07-10 Thread Amit Kapila
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.

2023-07-10 Thread Peter Smith
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.

2023-07-09 Thread Amit Kapila
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.

2023-07-09 Thread Peter Smith
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.

2023-07-07 Thread Amit Kapila
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.

2023-07-07 Thread Masahiko Sawada
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.

2023-07-06 Thread Peter Smith
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.

2023-07-05 Thread Masahiko Sawada
On Thu, Jul 6, 2023 at 7:58 AM Peter Smith wrote: > > On Wed, Jul 5, 2023 at 4:32 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.

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

2023-07-05 Thread Peter Smith
On Wed, Jul 5, 2023 at 4:32 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. > > > > > > -- > > > > > > I

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

2023-07-05 Thread Amit Kapila
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.

2023-07-05 Thread Masahiko Sawada
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. > > > > -- > > > > I wasn't sure about the code comment adjustments suggested by Amit [1]: > >

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

2023-07-04 Thread Amit Kapila
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. > > -- > > I wasn't sure about the code comment adjustments suggested by Amit [1]: > "Accordingly, the comments atop build_replindex_scan_key(), >

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

2023-07-04 Thread Peter Smith
Hi. Here are some review comments for this patch. +1 for the patch idea. -- I wasn't sure about the code comment adjustments suggested by Amit [1]: "Accordingly, the comments atop build_replindex_scan_key(), FindUsableIndexForReplicaIdentityFull(), IsIndexOnlyOnExpression() should also be

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

2023-07-04 Thread Amit Kapila
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 searching the rows.

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

2023-07-02 Thread Masahiko Sawada
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