I'm amazed we never caught this before ... - Mailing list pgsql-hackers

From Tom Lane
Subject I'm amazed we never caught this before ...
Date
Msg-id 5610.978825561@sss.pgh.pa.us
Whole thread Raw
Responses Re: I'm amazed we never caught this before ...
List pgsql-hackers
The stringToNode routines (backend/nodes/read.c and readfuncs.c) depend
on a static variable that is the next-input pointer for lsptok().

Guess what happens if stringToNode is invoked recursively.  (Hint:
it's not good.)

It's apparently not easy for this to happen, but I have a reproducible
test case involving relcache flush while reading the rule action
parsetree for a view.  readDatum() needs to be able to look up the
pg_type entry for the value it's reading, and that lookup could result
in relcache flush, which would lead to an attempt to read the rule
actions for any locked relcache entries --- like, say, the one we
are currently trying to load.

I propose fixing this by making stringToNode save and restore the
entry-time value of the static pointer.  (Alternatively, we could
explicitly pass a pointer-to-pointer-to-char to lsptok() and every
single one of the readfuncs, but that seems like way too much
notational clutter.)

I am not sure that is enough to fix the problem completely.  It might
be best to avoid calling get_typbyval() from readDatum, which could
be done if we re-ordered the fields of a CONST dump so that the byval
field appears before we need to read the const value itself.  That
means an initdb, but we've already forced an initdb for beta2 ...

Marc, can you hold off wrapping beta2 for a few hours?  I think this
is a 'must fix'.
        regards, tom lane


pgsql-hackers by date:

Previous
From: Tom Lane
Date:
Subject: Re: CVS regression test failure on OBSD
Next
From: The Hermit Hacker
Date:
Subject: Re: I'm amazed we never caught this before ...