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()
t; > 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
>
mments 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()?
Good point.
Looking at
entityFull().
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
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 see
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 not
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
ferences the remote
- * 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
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 c
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
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
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 Mas
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 PM
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 the
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
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 checked
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. W
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
> > IsIndexOnlyOnExpress
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 dr
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
> ...
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
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
> > >
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 Sawad
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 attach
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 I
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 lef
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: "Candi
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 column
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
d. I thought 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 th
sion() function 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.
> >
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.
> > >
> > > --
> > >
> > > I
esn't 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.
> > C
PK/ReplicaIdentity, 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.
ric 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 sid
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
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
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
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
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 generat
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 b_dr
column problem can't be detected.
>
>
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 s
I think we want to test 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
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 a
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 comments
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.
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 sav
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
> diff
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...
>
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 of
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:
v2
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 y
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
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)
>
> the 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
> &
ot;? 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
>
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 messa
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
when when the REPLICA IDENTITY FULL is used with dropped
There is an extra "when".
Regards,
Shi Yu
ed columns, 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
gene
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"; do \
On 12/4/17 22:15, Noah Misch wrote:
> While reviewing this patch, I noticed a couple of nearby defects:
>
> - RelationFindReplTupleSeq() says "Note that this stops on the first matching
> tuple.", but that's not the case. It visits every row in the table, and it
> uses the last match. The cl
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 thoughts about keeping datumAsEqual() as a
63 matches
Mail list logo
