Thread: Did someone break CVS?

I think it's that XLog stuff:

gcc -pipe -O -g -Wall -Wmissing-prototypes -Wmissing-declarations  -R/home/c
hriskl/local/lib -export-dynamic access/SUBSYS.o bootstrap/SUBSYS.o
catalog/SUBSYS.o parser/SUBSYS.o commands/SUBSYS.o executor/SUBSYS.o
lib/SUBSYS.o libpq/SUBSYS.o main/SUBSYS.o nodes/SUBSYS.o optimizer/SUBSYS.o
port/SUBSYS.o postmaster/SUBSYS.o regex/SUBSYS.o rewrite/SUBSYS.o
storage/SUBSYS.o tcop/SUBSYS.o
utils/SUBSYS.o -lz -lreadline -lcrypt -lcompat -lm -lutil  -o postgres
tcop/SUBSYS.o: In function `PostgresMain':
/home/chriskl/pgsql-head/src/backend/tcop/postgres.c:1550: undefined
reference to `XLogDir'
gmake[2]: *** [postgres] Error 1
gmake[2]: Leaving directory `/home/chriskl/pgsql-head/src/backend'
gmake[1]: *** [all] Error 2
gmake[1]: Leaving directory `/home/chriskl/pgsql-head/src'
gmake: *** [all] Error 2

Chris

> I think it's that XLog stuff:

Did you try a make clean? Did you update all of your directories? I
don't see a problem here, and I don't see any uncommitted files...
                   - Thomas

Thomas Lockhart <lockhart@fourpalms.org> writes:
>> I think it's that XLog stuff:
> Did you try a make clean? Did you update all of your directories? I
> don't see a problem here, and I don't see any uncommitted files...

I see the same failure in a fresh-this-morning CVS checkout: the final
backend link step fails with

tcop/SUBSYS.o: In function `PostgresMain':
/home/tgl/pgsql/src/backend/tcop/postgres.c:1550: undefined reference to `XLogDir'

Are you not building with --enable-cassert, perhaps?  The fault seems to
be due to "Assert(strlen(XLogDir) > 0)".  Also, there is still an extern
for XLogDir in access/xlog.h, which is bogus if it's going to be static.
        regards, tom lane

> >> I think it's that XLog stuff:
> Are you not building with --enable-cassert, perhaps?  The fault seems to
> be due to "Assert(strlen(XLogDir) > 0)".  Also, there is still an extern
> for XLogDir in access/xlog.h, which is bogus if it's going to be static.

I've updated xlog.h and xlog.c to fix the problem.

I've removed the arbitrary upper limit on the allowed size of the
directory path at the same time, though it was a reasonably large limit
so would not likely be a problem in practice.

Sometimes I compile with assert checking on, and sometimes not; didn't
check this one both ways.
                  - Thomas

Thomas Lockhart <lockhart@fourpalms.org> writes:
> I've removed the arbitrary upper limit on the allowed size of the
> directory path at the same time,

Oh?  Have you fixed *every* place that constrains the length limit of
an XLOG-derived file name?  Just making XLogDir dynamically allocated
will not improve matters, but IMHO make 'em worse because failures will
occur on-the-fly instead of at startup.

In general I think that removing MAX_PG_PATH limits is a rather
pointless exercise --- no one has yet complained that MAX_PG_PATH is too
small.
        regards, tom lane

> In general I think that removing MAX_PG_PATH limits is a rather
> pointless exercise --- no one has yet complained that MAX_PG_PATH is too
> small.

I just mentioned it in passing; that wasn't the point of the changes.

Is there a design pattern that would ask us to enforce that length
limit? If so, I'd be happy to do so; if not, it doesn't much matter...
                   - Thomas

Thomas Lockhart <lockhart@fourpalms.org> writes:
> Is there a design pattern that would ask us to enforce that length
> limit? If so, I'd be happy to do so; if not, it doesn't much matter...

Well, the issue is that the backend is just full of code like
   char        tmppath[MAXPGPATH];
   snprintf(tmppath, MAXPGPATH, "%s/xlogtemp.%d",            XLogDir, (int) getpid());

I suppose we could run around and try to replace every single such
occurrence with dynamically-sized buffers, but it seems hardly worth the
trouble --- and if you want a positive argument, I'd prefer not to
introduce another potential source of elogs (namely out-of-memory)
into code segments that run as critical sections, as some of the xlog
manipulation code does.  Any elog there becomes a database panic.  Is
it worth taking such a risk to eliminate a limit that *no one* has ever
complained about?

It would actually be better to limit XLogDir to MAXPGPATH minus a couple
dozen characters, to ensure that filenames formed in the style above
cannot overflow their buffer variables.

BTW: was there anything in that patch that ensured XLogDir would be
an absolute path?  A relative path is guaranteed not to work.
        regards, tom lane

> Thomas Lockhart <lockhart@fourpalms.org> writes:
>> Is there a design pattern that would ask us to enforce that length
>> limit? If so, I'd be happy to do so; if not, it doesn't much matter...
> 
> Well, the issue is that the backend is just full of code like
> 
>     char        tmppath[MAXPGPATH];
> 
>     snprintf(tmppath, MAXPGPATH, "%s/xlogtemp.%d",
>              XLogDir, (int) getpid());
> 
> I suppose we could run around and try to replace every single such
> occurrence with dynamically-sized buffers, but it seems hardly worth the
> trouble --- and if you want a positive argument, I'd prefer not to
> introduce another potential source of elogs (namely out-of-memory)
> into code segments that run as critical sections, as some of the xlog
> manipulation code does.  Any elog there becomes a database panic.  Is
> it worth taking such a risk to eliminate a limit that *no one* has ever
> complained about?

If that one person did exist, would it not be possible for them to just 
increase the value of MAXPGPATH and recompile?