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

On Wed, Aug 13, 2025 at 03:29:50PM +0530, Ashutosh Bapat wrote:

Sorry for the delay. This has been on my TODO list for some time.
Back to it now.

> On Wed, Aug 13, 2025 at 3:18 PM Ashutosh Bapat <ashutosh(dot)bapat(dot)oss(at)gmail(dot)com> wrote:
>> On Thu, Jun 19, 2025 at 2:17 PM Ashutosh Bapat <ashutosh(dot)bapat(dot)oss(at)gmail(dot)com> wrote:
>>> The description of UNLOGGED mentions "permanent" so it looks like your
>>> use of "permanent" covers both regular and unlogged tables. However
>>> looking purely at the RELPERSISTENCE_ names, it' s possible that one
>>> will associate the word "permanent" with RELPERSISTENCE_PERMANENT
>>> only, and not with RELPERSISTENCE_UNLOGGED. Should we use
>>> "non-temporary" instead to avoid confusion?

FWIW, I see a pretty good point in backpatching what we have here, as
active users of logical replication can be impacted by this issue
randomly. Even if this requires a wraparound, for some users it may
be a window that can open in a couple of days, mostly for workloads
with a high temp table exhaustion, I guess. In short, let's not touch
pg_proc.dat, and I'm planning for a backpatch all the way down due to
the possible random intrusions with the logirep paths.

The changes at the top of RelidByRelfilenumber() describing the
problem with temporary tables feels a bit bloated. I see no need to
mention directly autoprewarm and logical replication, two of the three
callers of this routine, with the explanation coming down to the fact
that this is only a temp relation problem because we cannot pinpoint
to an OID without knowing a proc number. So let's cut a few lines
here at the top of RelidByRelfilenumber(), let's add a simpler line in
func-admin.sgml to say directly and only that "temporary relations are
not supported", passing on the "permanent" vs "no permanent" business.
I don't see a direct need to mention that at the top of
pg_relation_filenode(), as long as RelidByRelfilenumber(), but OK by
me to add a simpler "temporary relations are not supported", it looks
like most prefer than based on the contents of the thread.

>>> Logical replication and autoprewarm may not cause such a large bloat
>>> but there is no limit to passing invalid combinations of reltablespace
>>> and relfilenumber to pg_filenode_relation(). Do we want to prohibit
>>> that case by passing a flag from logical pg_filenode_relation() to not
>>> cache invalid entries?

Fun. Yes, I agree that we could do better here. It does not strike
me as an issue as invasive as the original report, direct calls of
pg_filenode_relation() are much rarer than the code paths of
autoprewarm and logical replication touched by RelidByRelfilenumber().
That's a potential HEAD improvement to me.

> Please ignore the 0002 patch in the earlier patchset. It's an
> experimental change related to negative entries bloat in
> RelfilenumberMap cache.

And I was wondering what was going on in the latest patch set.. I'm
ignoring this part.
--
Michael