From: | Amit Langote <amitlangote09(at)gmail(dot)com> |
---|---|
To: | Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> |
Cc: | "shiy(dot)fnst(at)fujitsu(dot)com" <shiy(dot)fnst(at)fujitsu(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, "houzj(dot)fnst(at)fujitsu(dot)com" <houzj(dot)fnst(at)fujitsu(dot)com> |
Subject: | Re: Replica Identity check of partition table on subscriber |
Date: | 2022-06-16 11:54:41 |
Message-ID: | CA+HiwqHK0j2JULoKb49LhNnumUAnteDUTe1mCmz7Z1PvxNuJUg@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Thu, Jun 16, 2022 at 6:42 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> On Fri, Jun 10, 2022 at 2:26 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
> > @@ -1735,6 +1735,13 @@ apply_handle_insert_internal(ApplyExecutionData *edata,
> > static void
> > check_relation_updatable(LogicalRepRelMapEntry *rel)
> > {
> > + /*
> > + * If it is a partitioned table, we don't check it, we will check its
> > + * partition later.
> > + */
> > + if (rel->localrel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
> > + return;
> >
> > Why do this? I mean why if logicalrep_check_updatable() doesn't care
> > if the relation is partitioned or not -- it does all the work
> > regardless.
> >
> > I suggest we don't add this check in check_relation_updatable().
>
> I think based on this suggestion patch has moved this check to
> logicalrep_rel_mark_updatable(). For a partitioned table, it won't
> even validate whether it can mark updatable as false which seems odd
> to me even though there might not be any bug due to that. Was your
> suggestion actually intended to move it to
> logicalrep_rel_mark_updatable?
No, I didn't intend to suggest that we move this check to
logicalrep_rel_mark_updatable(); didn't notice that that's what the
latest patch did.
What I said is that we shouldn't ignore the updatable flag for a
partitioned table in check_relation_updatable(), because
logicalrep_rel_mark_updatable() would have set the updatable flag
correctly even for partitioned tables. IOW, we should not
special-case partitioned tables anywhere.
I guess the point of adding the check is to allow the case where a
leaf partition's replica identity can be used to apply an update
originally targeting its ancestor that doesn't itself have one.
I wonder if it wouldn't be better to move the
check_relation_updatable() call to
apply_handle_{update|delete}_internal()? We know for sure that we
only ever get there for leaf tables. If we do that, we won't need the
relkind check.
--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com
From | Date | Subject | |
---|---|---|---|
Next Message | Michael Paquier | 2022-06-16 11:59:43 | Re: PGDOCS - Integer configuration parameters should say "(integer)" |
Previous Message | vignesh C | 2022-06-16 10:23:18 | Re: Handle infinite recursion in logical replication setup |