I'm amazed we never caught this before ...

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: The Hermit Hacker <scrappy(at)hub(dot)org>
Cc: pgsql-hackers(at)postgreSQL(dot)org
Subject: I'm amazed we never caught this before ...
Date: 2001-01-06 23:59:21
Message-ID: 5610.978825561@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: 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

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message The Hermit Hacker 2001-01-07 00:02:36 Re: I'm amazed we never caught this before ...
Previous Message Tom Lane 2001-01-06 23:45:44 Re: CVS regression test failure on OBSD