Re: Useless code in RelationCacheInitializePhase3

Andres Freund <andres(at)anarazel(dot)de> writes:
> On 2019-04-12 14:17:11 -0400, Tom Lane wrote:
>> While looking at the pending patch to clean up management of
>> rd_partcheck, I noticed that RelationCacheInitializePhase3 has code that
>> purports to reload rd_partkey and rd_partdesc, but none for rd_partcheck.
>> However, that reload code is dead code, as is easily confirmed by
>> checking the code coverage report, because we have no partitioned system
>> catalogs.
>>
>> Moreover, if somebody tried to add such a catalog, I'd bet a good deal
>> of money that this code would not work. It seems highly unlikely that
>> we could run RelationBuildPartitionKey or RelationBuildPartitionDesc
>> successfully when we haven't even finished bootstrapping the relcache.

> But it sure would be nice if we made it work at some point.

Whether it would be nice or not is irrelevant to my point: this code
doesn't work, and it's unlikely that it would ever be part of a working
solution. I don't think there's any way that it'd be sane to attempt
catalog accesses during RelationCacheInitializePhase3. If we want any
of these features for system catalogs, I think the route to a real fix
would be to make them load-on-demand data so that they can be fetched
later on. Or, possibly, the easiest way is to include these data
structures in the dumped cache file. But what's here is a dead end.
I'd even call it an attractive nuisance, because it encourages people
to add yet more nonfunctional code, rather than pointing them in the
direction of doing something useful.

>> I am less sure about whether the table-access-method stanza is as silly
>> as the rest, but I do see that it's unreached in current testing.
>> So I wonder whether there is any thought that we'd realistically support
>> catalogs with nondefault AMs, and if there is, does anyone think that
>> this code would work?

> Right now it definitely won't work,

Sure, I wasn't expecting that. The question is the same as above:
is it plausible that this code would appear in this form in a complete
working implementation? If not, I think we should rip it out rather
than leave the impression that we think it does something useful.

> I think it probably would work for catalog tables, as it's coded right
> now. There's no catalog lookups RelationInitTableAccessMethod() for
> tables that return true for IsCatalogTable(). In fact, I think we should
> apply something like:

Makes sense, and I'd also add some comments pointing out that there had
better not be any catalog lookups when this is called for a system
catalog.

regards, tom lane