Re: including backend ID in relpath of temp rels - updated patch - Mailing list pgsql-hackers

Robert Haas <robertmhaas@gmail.com> writes:
> Here's an updated patch, with the invalidation changes merged in and
> hopefully-suitable adjustments elsewhere.

I haven't tested this patch, but I read through it (and have I mentioned
how unbelievably illegible -u format patches are?).  It seems to be
close to commitable, but I found a few issues.  In no particular order:

storage.sgml needs to be updated to describe this file-naming scheme.

BackendRelFileNode isn't a particularly nice struct name; it seems like
it puts the most important aspect of the struct's purpose somewhere in
the middle of the name.  Maybe RelFileNodeBackend would be better, or
RelFileNodeFull, or something like that.

In GetNewRelFileNode, you've changed some code that is commented
/* This logic should match RelationInitPhysicalAddr */, but there
is not any corresponding change in RelationInitPhysicalAddr.  I find
this bothersome.  I think the problem is that you've made it look
like the assignment of rnode.backend is part of the logic that ought
to match RelationInitPhysicalAddr.  Perhaps set that off, at least
by a blank line, if not its own comment.

The relpath() and relpathperm() macros are a tad underdocumented for my
taste; at least put comments noting that one takes a RelFileNode and the
other a BackendRelFileNode.  And I wonder if you couldn't find better
names for relpathperm and relpathbackend.

assign_maxconnections and assign_autovacuum_max_workers need to be fixed
for MAX_BACKENDS; in fact I think all the occurrences of INT_MAX / 4 in
guc.c need to be replaced.  (And if I were you I'd grep to see if
anyplace outside guc.c knows that value...)  Also, as a matter of style,
the comment currently attached to max_connections needs to be attached
to the definition of the constant, not just modified where it is.

As an old bit-shaver this sorta bothers me:

+++ b/src/include/utils/rel.h
@@ -127,7 +127,7 @@ typedef struct RelationData    struct SMgrRelationData *rd_smgr;    /* cached file handle, or NULL
*/   int            rd_refcnt;        /* reference count */    bool        rd_istemp;        /* rel is a temporary
relation*/
 
-    bool        rd_islocaltemp; /* rel is a temp rel of this session */
+    BackendId    rd_backend;        /* owning backend id, if temporary relation */    bool        rd_isnailed;    /*
relis nailed in cache */    bool        rd_isvalid;        /* relcache entry is valid */    char        rd_indexvalid;
 /* state of rd_indexlist: 0 = not valid, 1 =
 

The struct is going to be less efficiently packed with that field order.
Maybe swap rd_istemp and rd_backend?

+        Assert(relation->rd_backend != InvalidOid);
ought to be InvalidBackendId, no?  Both new calls of GetTempNamespaceBackendId
have this wrong.  You might also want to change the comment of
GetTempNamespaceBackendId to note that its failure result is not just -1
but InvalidBackendId.

Lastly, it bothers me that you've put in code to delete files belonging
to temp rels during crash restart, without any code to clean up their
catalog entries.  This will therefore lead to dangling pg_class
references, with uncertain but probably not very nice consequences.
I think that probably shouldn't go in until there's code to drop the
catalog entries too.
        regards, tom lane