Re: found xmin from before relfrozenxid on pg_catalog.pg_authid

Hi,

On 2018-05-28 12:52:06 -0700, Andres Freund wrote:
> Hi,
> 
> On 2018-05-27 13:00:06 -0700, Andres Freund wrote:
> > I've a patch that seems to work, that mostly needs some comment
> > polishing.
> 
> Attached is what I currently have. Still needs some more work, but I
> think it's more than good enough to review the approach.  Basically the
> approach consists out of two changes:
> 
> 1) Send init file removals for shared nailed relations as well.
> 
>    This fixes that the shared init file contains arbitrarily outdated
>    information for relfrozenxid etc. Leading to e.g. the pg_authid
>    errors we've seen in some recent threads.  Only applies to
>    new connections.
> 
> 2) Reread RelationData->rd_rel for nailed relations when invalidated.
> 
>    This ensures that already built relcache entries for nailed relations
>    are updated. Currently they never are.  This currently doesn't cause
>    *that* frequently an issue for !shared entries, because for those the
>    init file gets zapped regularly, and autovacuum workers usually don't
>    live that long.  But it's still a significant correctness issue for
>    both shared an non shared relations.

Here's a more polished v2 version of the patch.  I, locally, did the
work to backpatch the change. Besides trivialities there's two
nontrivial changes:

- In earlier versions there's no global invalidations. I've inquired and
  complained about what exactly they're for in
  http://archives.postgresql.org/message-id/20180611231634.w2rgtlzxaw4loefk%40alap3.anarazel.de

  In earlier branches we just don't do the global thing. That seems
  unproblematic to me.

- The bigger issue is that in 9.3, and just there
  https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=8de3e410faa06ab20ec1aa6d0abb0a2c040261ba
  does not yet exist.

  That means back then we performed reloads even outside a
  transaction. I don't feel confident about invoking additional catalog
  reloads in the new situations, so I kept the IsTransactionState()
  checks in RelationReloadNailed().  That seems less risky, but possibly
  somebody wants to argue the other way round?

There's some minor other conflicts, but they're all pretty obvious.

I plan to go over the change again tomorrow, and then push. Unless
somebody has comments before then, obviously.


> FWIW, I wonder if this isn't critical enough to make us consider having
> a point release earlier..

Still think this is something we should seriously consider.

- Andres