So I was the victim assigned to review this patch.
The code is pretty much impeccable as usual from Noah. But I have
questions about the semantics of it.
Firstly this bit makes me wonder:
+ /* Not-found is always final. */
+ if (!OidIsValid(relOid1))
+ return relOid1;
If someone does
BEGIN; DROP TABLE foo; CREATE TABLE foo; COMMIT;
Then what prevents this logic from finding the doomed relation,
blocking until the transaction commits, then finding it's deleted and
returning InvalidOid?
RangeVarGetRelid is just going to complete its index scan of pg_class
and may not come across the newly inserted row.
Am I missing something? I would have expected to have to loop around
and retry if no valid record is found. But this raises the question --
if no lock was acquired then what would have triggered an
AcceptInvalidatationMessages and how would we know we waited long
enough to find out about the newly created table?
As a side note, if there are a long stream of such concurrent DDL then
this code will leave all the old versions locked. This is consistent
with our "hold locks until end of transaction" semantics but it seems
weird for tables that we locked "accidentally" and didn't really end
up using at all. I'm not sure it's really bad though.