SEO

Hi,

I think I have found a small problem in pg_regress.

In convert_sourcefiles() it stats directories based on the current
working directory, but in convert_sourcefiles_in() it reads files based
in srcdir or abs_builddir.

The attached patch changes the behavior of pg_regress, so that it stats
the directories that it read the files from. This patch will also make
pg_regress fail if the directories are missing, instead of silently
ignoring the missing directories. If you think this fail-fast behavior
is undesirable, I can create a patch that just ignores the files (todays
behavior), or ignores them with a warning.

The attached patch is tested by running pg_regress in a non-standard
path and by running "make check", both on Solaris Nevada on x64.

-J
--

Jørgen Austvik, Software Engineering - QA
Sun Microsystems Database Technology Group



---------------------------(end of broadcast)---------------------------
TIP 5: don't forget to increase your free space map settings

Jorgen Austvik - Sun Norway wrote:

> This patch will also make pg_regress fail if the directories are
> missing, instead of silently ignoring the missing directories. If you
> think this fail-fast behavior is undesirable, I can create a patch
> that just ignores the files (todays behavior), or ignores them with a
> warning.

This silent-skip behavior is there to avoid noise in other tests. Try
the ecpg tests, contrib tests and src/pl tests.

I didn't check the rest of the patch, but did you verify that it still
works in VPATH builds?

--
Alvaro Herrera http://www.advogato.org/person/alvherre
"Nunca confiaré en un traidor. Ni siquiera si el traidor lo he creado yo"
(Barón Vladimir Harkonnen)

---------------------------(end of broadcast)---------------------------
TIP 2: Don't 'kill -9' the postmaster

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> I didn't check the rest of the patch, but did you verify that it still
> works in VPATH builds?

Actually, it looks to me like the patch is wrong specifically because
it does not do the right thing in the VPATH case.

regards, tom lane

---------------------------(end of broadcast)---------------------------
TIP 5: don't forget to increase your free space map settings

Tom Lane wrote:
> Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
>> I didn't check the rest of the patch, but did you verify that it still
>> works in VPATH builds?
>
> Actually, it looks to me like the patch is wrong specifically because
> it does not do the right thing in the VPATH case.

Are you thinking about "failing if the folders are missing" as "not the
right thing in the VPATH case", or are you thinking about something else?

-J
--
Jørgen Austvik, Software Engineering
Database Technology Group


---------------------------(end of broadcast)---------------------------
TIP 3: Have you checked our extensive FAQ?

http://www.postgresql.org/docs/faq

Alvaro Herrera wrote:
> Jorgen Austvik - Sun Norway wrote:
>
>> This patch will also make pg_regress fail if the directories are
>> missing, instead of silently ignoring the missing directories. If you
>> think this fail-fast behavior is undesirable, I can create a patch
>> that just ignores the files (todays behavior), or ignores them with a
>> warning.
>
> This silent-skip behavior is there to avoid noise in other tests. Try
> the ecpg tests, contrib tests and src/pl tests.

OK, thank you for the pointers, I will look into this.

I will send out a new patch that is tested with those, and that does the
silent-skip.

> I didn't check the rest of the patch, but did you verify that it still
> works in VPATH builds?

I saw two comments in the file mentioning VPATH, but I have no idea
about what that is. Based on reading the code, I belive this patch
should work better with VPATH (setting srcpath) than without, as it
actually checks those folders instead of other folders.

-J
--
Jørgen Austvik, Software Engineering
Database Technology Group

---------------------------(end of broadcast)---------------------------
TIP 7: You can help support the PostgreSQL project by donating at

http://www.postgresql.org/about/donate

Jorgen Austvik <Jorgen.Austvik@Sun.COM> writes:
> Tom Lane wrote:
>> Actually, it looks to me like the patch is wrong specifically because
>> it does not do the right thing in the VPATH case.

> Are you thinking about "failing if the folders are missing" as "not the
> right thing in the VPATH case", or are you thinking about something else?

The point is that in VPATH you are running in a build tree, and should
copy the source files from the source tree, but *not* modify the source
tree. Thus, fetching relative to $srcdir but writing relative to .
is in fact the correct behavior.

There has not previously been any complaint that pg_regress was broken
in this regard, so maybe you should take two steps back and explain what
problem you think needs fixing, rather than just dropping a patch on us.

regards, tom lane

---------------------------(end of broadcast)---------------------------
TIP 1: if posting/reading through Usenet, please send an appropriate
subscribe-nomail command to majordomo@postgresql.org so that your
message can get through to the mailing list cleanly

Tom Lane wrote:
> Jorgen Austvik <Jorgen.Austvik@Sun.COM> writes:
>> Tom Lane wrote:
>>> Actually, it looks to me like the patch is wrong specifically because
>>> it does not do the right thing in the VPATH case.
>
>> Are you thinking about "failing if the folders are missing" as "not the
>> right thing in the VPATH case", or are you thinking about something else?
>
> The point is that in VPATH you are running in a build tree, and should
> copy the source files from the source tree, but *not* modify the source
> tree. Thus, fetching relative to $srcdir but writing relative to .
> is in fact the correct behavior.

Ah, I understand.

It is this part you don't like:
---8<--------------8<--------------8<--------------8<--------------8<-----------
+ snprintf(destdir, MAXPGPATH, "%s/%s", abs_srcdir, dest);
<snip>
! snprintf(destfile, MAXPGPATH, "%s/%s.%s", destdir, prefix, suffix);
---8<--------------8<--------------8<--------------8<--------------8<-----------

Thanks for the guidance, I'll try come up with a better alternative.
Should outputdir be used instead of current working directory (cwd) if
it is set?

> There has not previously been any complaint that pg_regress was broken
> in this regard, so maybe you should take two steps back and explain what
> problem you think needs fixing, rather than just dropping a patch on us.

I tried to explain it in the mail, but let me try again, this time
showing some code.

Here we stat <cwd>/input:

---8<--------------8<--------------8<--------------8<--------------8<-----------
static void
convert_sourcefiles(void)
{
struct stat st;
int ret;

ret = stat("input", &st);
if (ret == 0 && S_ISDIR(st.st_mode))
convert_sourcefiles_in("input", "sql", "sql");
---8<--------------8<--------------8<--------------8<--------------8<-----------

But if we have set srcdir, the directory we are stat'ing, is not the
same directory that we are reading the files from:

---8<--------------8<--------------8<--------------8<--------------8<-----------
static void
convert_sourcefiles_in(char *source, char *dest, char *suffix)

<snip>

if (srcdir)
strcpy(abs_srcdir, srcdir);
else
strcpy(abs_srcdir, abs_builddir);

snprintf(indir, MAXPGPATH, "%s/%s", abs_srcdir, source);
names = pgfnames(indir);
---8<--------------8<--------------8<--------------8<--------------8<-----------

So I wanted to provide a patch that ran stat on the folder that we were
reading the files from.

-J
--
Jørgen Austvik, Software Engineering
Database Technology Group


---------------------------(end of broadcast)---------------------------
TIP 6: explain analyze is your friend

Jorgen Austvik <Jorgen.Austvik@Sun.COM> writes:
> But if we have set srcdir, the directory we are stat'ing, is not the
> same directory that we are reading the files from:

Ah. The reason this works in VPATH mode is that setup of the build tree
duplicated all subdirectories of the source tree, so ./input/ should
exist iff $srcdir/input/ does. I agree it's a bit ugly though; it'd
be better to stat what we really plan to read.

Maybe push the stat operation inside convert_sourcefiles_in ?

regards, tom lane

---------------------------(end of broadcast)---------------------------
TIP 5: don't forget to increase your free space map settings

Tom Lane wrote:
> Ah. The reason this works in VPATH mode is that setup of the build tree
> duplicated all subdirectories of the source tree, so ./input/ should
> exist iff $srcdir/input/ does. I agree it's a bit ugly though; it'd
> be better to stat what we really plan to read.
>
> Maybe push the stat operation inside convert_sourcefiles_in ?

Yes, that was what I tried to do in the patch, but unfortunately I was
too eager I'll send a new proposal tomorrow. Thanks for your help!

-J
--
Jørgen Austvik, Software Engineering
Database Technology Group


---------------------------(end of broadcast)---------------------------
TIP 3: Have you checked our extensive FAQ?

http://www.postgresql.org/docs/faq

Jorgen Austvik wrote:
>> Maybe push the stat operation inside convert_sourcefiles_in ?
>
> Yes, that was what I tried to do in the patch, but unfortunately I was
> too eager I'll send a new proposal tomorrow. Thanks for your help!

Hi,

this patch should only move the stat operation into
convert_sourcefiles_in().

The attached patch is tested by running pg_regress in a non-standard
path, and by running "make check" in pgsql plus in ecpg.

-J
--

Jørgen Austvik, Software Engineering - QA
Database Technology Group


---------------------------(end of broadcast)---------------------------
TIP 2: Don't 'kill -9' the postmaster