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

> I have just committed changes that address the problem of relcache
> entries not being updated promptly after another backend issues
> a shared-invalidation report.  LockRelation() now checks for sinval
> reports just after acquiring a lock, and the relcache entry will be
> updated if necessary --- or, if the relation has actually disappeared
> entirely, an elog(ERROR) will occur.
> 
> As a side effect of the relcache update, the underlying md.c/fd.c files
> will be closed, and then reopened if necessary.  This should solve our
> concerns about vacuum.c not being able to truncate relations safely.
> 
> There is still some potential for misbehavior as a result of the fact
> that the parser looks at relcache entries without bothering to obtain
> any kind of lock on the relation.  For example:
> 
> -- In backend #1:
> regression=> create table z1 (f1 int4);
> CREATE
> regression=> select * from z1;
> f1
> --
> (0 rows)
> 
> regression=> begin;
> BEGIN
> 
> -- In backend #2:
> regression=> alter table z1 add column f2 int4;
> ADD
> regression=>
> 
> -- In backend #1:
> regression=> select * from z1;
> f1
> --
> (0 rows)
> 
> -- parser uses un-updated relcache entry and sees only one column in z1.
> -- However, the relcache *will* get updated as soon as we either lock a
> -- table or do the CommandCounterIncrement() at end of query, so a second
> -- try sees the new info:
> regression=> select * from z1;
> f1|f2
> --+--
> (0 rows)
> 
> regression=> end;
> END
> 
> The window for problems is pretty small: you have to be within a
> transaction (otherwise the StartTransaction will notice the sinval
> report), and your very first query after the other backend does
> ALTER TABLE has to reference the altered table.  So I'm not sure
> this is worth worrying about.  But perhaps the parser ought to obtain
> the weakest possible lock on each table referenced in a query before
> it does any looking at the attributes of the table.  Comments?

Ok. I will give another example that seems more serious.

test=> begin;
BEGIN
test=> create table t1(i int);
CREATE
-- a table file named "t1" is created.
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*
test=> end;
END
test=> create table t1(i int); 
ERROR:  cannot create t1
-- since relation file t1 already exists.
test=> 
EOF
[t-ishii@ext16 src]$ !!
psql test
Welcome to the POSTGRESQL interactive sql monitor: Please read the file COPYRIGHT for copyright terms of POSTGRESQL
[PostgreSQL 6.6.0 on powerpc-unknown-linux-gnu, compiled by gcc egcs-2.90.25 980302 (egcs-1.0.2 prerelease)]
  type \? for help on slash commands  type \q to quit  type \g or terminate with semicolon to execute queryYou are
currentlyconnected to the database: test
 

test=> select * from t1;

ERROR:  t1: Table does not exist.
test=> create table t1(i int);

ERROR:  cannot create t1
-- again, since relation file t1 already exists.
-- user would never be able to create t1!

I think the long range solution would be let parser obtain certain
locks as Tom said. 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.

---------------------------- backtrace -----------------------------
#0  mdopen (reln=0x1a9af18) at md.c:279
#1  0x18cb784 in smgropen (which=425, reln=0xbfffdef0) at smgr.c:185
#2  0x18cb784 in smgropen (which=0, reln=0x1a9af18) at smgr.c:185
#3  0x1904c1c in RelationBuildDesc ()
#4  0x1905360 in RelationNameGetRelation ()
#5  0x18259a4 in heap_openr ()
#6  0x187f59c in addRangeTableEntry ()
#7  0x1879cb0 in transformTableEntry ()
#8  0x1879d40 in parseFromClause ()
#9  0x1879a90 in makeRangeTable ()
#10 0x1871fd8 in transformSelectStmt ()
#11 0x1870d14 in transformStmt ()
#12 0x18709e0 in parse_analyze ()
#13 0x18792d4 in parser ()
#14 0x18cd158 in pg_parse_and_plan ()
#15 0x18cd5c0 in pg_exec_query_dest ()
#16 0x18cd524 in pg_exec_query ()
#17 0x18ce9ac in PostgresMain ()
#18 0x18a5994 in DoBackend ()
#19 0x18a53c8 in BackendStartup ()
#20 0x18a46d0 in ServerLoop ()
#21 0x18a4108 in PostmasterMain ()
#22 0x1870928 in main ()
---------------------------- backtrace -----------------------------

---------------------------- patches  -----------------------------
*** md.c~    Sun Sep  5 08:41:28 1999
--- md.c    Sun Sep  5 11:01:57 1999
***************
*** 286,296 ****
--- 286,303 ----      /* this should only happen during bootstrap processing */     if (fd < 0)
+     {
+         if (!IsBootstrapProcessingMode())
+         {
+             elog(ERROR,"Couldn't open %s\n", path);
+             return -1;
+         } #ifndef __CYGWIN32__         fd = FileNameOpenFile(path, O_RDWR | O_CREAT | O_EXCL, 0600); #else         fd
=FileNameOpenFile(path, O_RDWR | O_CREAT | O_EXCL | O_BINARY, 0600); #endif
 
+     }      vfd = _fdvec_alloc();     if (vfd < 0)