Re: [HACKERS] md.c is feeling much better now, thank you

Bruce Momjian <maillist(at)candle(dot)pha(dot)pa(dot)us> writes:
> Any resolution on this?

I believe I committed Tatsuo's change.

There is still the issue that the parser doesn't obtain any lock on
a relation during parsing, so it's possible to use a slightly stale
relcache entry for parsing purposes. (It can't be *really* stale,
since presumably we just read the SI queue during StartTransaction,
but still it could be wrong if someone commits an ALTER TABLE while
we are parsing our query.)

After thinking about it for a while, I am not sure if we should try to
fix this or not. The obvious fix would be to have the parser grab
AccessShareLock on any relation as soon as it is seen in the query,
and then keep this lock till end of transaction; that would guarantee
that no one else could alter the table structure and thereby invalidate
the parser's information about the relation. But that does not work
because it guarantees deadlock if two processes both try to get
AccessExclusiveLock, as in plain old "BEGIN; LOCK TABLE table; ...".
They'll both be holding AccessShareLock so neither can get exclusive.

There might be another way, but we need to be careful not to choose
a cure that's worse than the disease.

regards, tom lane

>> Tatsuo Ishii <t-ishii(at)sra(dot)co(dot)jp> writes:
>>>> Ok. I will give another example that seems more serious.
>>>> test=> aaa;
>>>> ERROR: parser: parse error at or near "aaa"
>>>> -- transaction is aborted and the table file t1 is unlinked.
>>>> test=> select * from t1;
>>>> -- but parser doesn't know t1 does not exist any more.
>>>> -- it tries to open t1 using mdopen(). (see including backtrace)
>>>> -- mdopen() tries to open t1 and fails. In this case mdopen()
>>>> -- creates t1!
>>>> NOTICE: (transaction aborted): queries ignored until END
>>>> *ABORT STATE*
>>
>> Hmm. It seems a more straightforward solution would be to alter
>> pg_parse_and_plan so that the parser isn't even called if we have
>> already failed the current transaction; that is, the "queries ignored"
>> test should occur sooner. I'm rather surprised to realize that
>> we do run the parser in this situation...
>>
>>>> I think the long range solution would be let parser obtain certain
>>>> locks as Tom said.
>>
>> That would not solve this particular problem, since the lock manager
>> wouldn't know any better than the parser that the table doesn't really
>> exist any more.
>>
>>>> Until that I propose following solution. It looks
>>>> simple, safe and would be neccessary anyway (I don't know why that
>>>> check had not been implemented). See included patches.
>>
>> This looks like it might be a good change, but I'm not quite as sure
>> as you are that it won't have any bad effects. Have you tested it?