Re: "unexpected duplicate for tablespace" problem in logical replication

On Tue, 19 Apr 2022 at 11:54, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> On Tue, Apr 12, 2022 at 11:45 AM Kyotaro Horiguchi
> <horikyota(dot)ntt(at)gmail(dot)com> wrote:
> >
> > At Mon, 11 Apr 2022 10:24:26 +0000, "wangsh(dot)fnst(at)fujitsu(dot)com" <wangsh(dot)fnst(at)fujitsu(dot)com> wrote in
> > > Please see the attachment
> >
> > Sorry for being late, but this seems to be in the wrong direction.
> >
> > In the problem case, as you know, GetNewRelFileNode does *not* check
> > oid uniqueness in pg_class. The on-catalog duplicate check is done
> > only when the result is to be used as the *table oid* at creation. In
> > other cases it chooses a relfilenode that does not duplicate in
> > storage file names irrelevantly to the relation oid. As the result
> > Non-temporary and temporary tables have separate relfilenode oid
> > spaces, either intentional or not..
> >
> > Thus, I think we don't want GetNewRelFileNode to check for duplicate
> > oid on pg_class if pg_class is not passed since that significantly
> > narrow the oid space for relfilenode. If we did something like that,
> > we might do check the existence of file names of the both non-temp and
> > temp in GetNewRelFileNode(), but that also narrows the relfilenode oid
> > space.
>
> Agreed.
>
> >
> > RelidByRelfilenode is called by autoprewarm, logical replication, and
> > pg_filenode_relation. In the autoprewarm and logical replication
> > cases, it is called only for non-temprary relations so letting the
> > function ignore (oid duplicating) temp tables works.
> > pg_relfilienode_relation is somewhat dubious. It is affected by this
> > bug. In the attached PoC, to avoid API change (in case for
> > back-patching), RelidByRelfilenode ignores duplcates of differernt
> > persistence. Also the PoC prioritize on persistent tables to
> > temporary ones but I'm not sure it's good decision, but otherwise we
> > need to change the signature of pg_filenode_relation.
> >
> > The attached is an PoC of that. The first one is a reproduction-aid
> > code. With it applied, the following steps create duplicate
> > relfilenode relations.
> >
> > What do you think about this direction?
>
> Sounds good to me. If we go in this direction, probably we also need
> to change the cache checks in RelidByRelfilenode() so that we
> prioritize non-temporary relations. Otherwise, we will end up
> returning the OID of temporary relation if it's already cached.
>
> Another idea I came up with is that we have RelidByRelfilenode() check
> only non-temporary relations by adding relpersistence !=
> RELPERSISTENCE_TEMP to the scan key. If we want to support temporary
> relations too, I think that, in v16, we can add relpersistence to
> RelidByRelfilenode() as a function argument (and a flag to
> pg_filenode_relation() accordingly) so that the user can get the name
> of the temporary relation by like pg_filenode_relation(0, 16386,
> true). On the other hand, in back branches, if an application is using
> pg_filenode_relation() to get the name of temporary relations, it
> would break it.

I have changed the status to "Waiting on Author" as the points raised
by Masahiko Sawada-san is pending.

Regards,
Vignesh