pgsql: Unify some tar functionality across different parts

On 01/01/2013 12:25 PM, Magnus Hagander wrote:

Unify some tar functionality across different parts

Move some of the tar functionality that existed mostly duplicated
in both pg_dump and the walsender basebackup functionality into
port/tar.c instead, so it can be used from both. It will also be
used by pg_basebackup in the future, which would've caused a third
copy of it around.

This seems to have broken plperl builds on Windows. Not sure what the
fix is.

cheers

andrew

--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

On Wed, Jan 2, 2013 at 4:14 AM, Andrew Dunstan <andrew@dunslane.net> wrote:

On 01/01/2013 12:25 PM, Magnus Hagander wrote:

Unify some tar functionality across different parts

Move some of the tar functionality that existed mostly duplicated
in both pg_dump and the walsender basebackup functionality into
port/tar.c instead, so it can be used from both. It will also be
used by pg_basebackup in the future, which would've caused a third
copy of it around.

This seems to have broken plperl builds on Windows. Not sure what the fix
is.

It seems if plperl exists it does *not* have the uid_t and gid_t datatypes?
port.h has this:

/*
* Supplement to <sys/types.h>.
*
* Perl already has typedefs for uid_t and gid_t.
*/
#ifndef PLPERL_HAVE_UID_GID
typedef int uid_t;
typedef int gid_t;
#endif

So either that's not true anymore, or we're including things in the wrong
order, I think.

I'm not sure where those are supposed to come from - they're not mentioned
anywhere in plperl. But maybe they're leaking in from the global perl
headers?

We could fix this by just changing the parameters to the tarCreateHeader()
function to take int instead of uid_t, but that seems like the wrong fix to
me. We should at least try to figure out why it's happening in the first
place. What happens if you just remove that #ifndef on a win build with
perl (sorry, don't have one around at the time - so if you have one, it
would me much helpful if you could test it) - do we get a different error,
or have we changed around the code and just forgotten that ifdef?

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

On Wed, Jan 2, 2013 at 10:06 AM, Magnus Hagander <magnus@hagander.net> wrote:

On Wed, Jan 2, 2013 at 4:14 AM, Andrew Dunstan <andrew@dunslane.net> wrote:

On 01/01/2013 12:25 PM, Magnus Hagander wrote:

Unify some tar functionality across different parts

Move some of the tar functionality that existed mostly duplicated
in both pg_dump and the walsender basebackup functionality into
port/tar.c instead, so it can be used from both. It will also be
used by pg_basebackup in the future, which would've caused a third
copy of it around.

This seems to have broken plperl builds on Windows. Not sure what the fix
is.

It seems if plperl exists it does *not* have the uid_t and gid_t datatypes?
port.h has this:

/*
* Supplement to <sys/types.h>.
*
* Perl already has typedefs for uid_t and gid_t.
*/
#ifndef PLPERL_HAVE_UID_GID
typedef int uid_t;
typedef int gid_t;
#endif

So either that's not true anymore, or we're including things in the wrong
order, I think.

I'm not sure where those are supposed to come from - they're not mentioned
anywhere in plperl. But maybe they're leaking in from the global perl
headers?

We could fix this by just changing the parameters to the tarCreateHeader()
function to take int instead of uid_t, but that seems like the wrong fix to
me. We should at least try to figure out why it's happening in the first
place. What happens if you just remove that #ifndef on a win build with perl
(sorry, don't have one around at the time - so if you have one, it would me
much helpful if you could test it) - do we get a different error, or have we
changed around the code and just forgotten that ifdef?

Got an env working.

So it is an ordering issue. If I remove that #ifdef, I get an error
that the perl headers are redefining uid_t and gid_t to the same thing
that they already are. So basically our problem is that the perl
headers are polluting the namespace, and our old workaround doesn't
work due to include ordering.

The problem is that this part of port/win32.h relies on it being
included first, and then the perl headers, before anybody can use
uid_t. Which means we can't use it anywhere in our own headers, which
is what causes this problem.

If it was a #define, we could just #undef it before including the perl
headers. But since it's a typedef, we can't.

I'm not really sure what the best thing is to do here. We can either
use the fact that we know that uid_t and gid_t are "int" on all our
platforms, more or less, and change the tar api to use uid_t. Or put
the tar functions in their own header and make sure we don't include
that one anywhere that we include the perl headers.

Thoughts?

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

Magnus Hagander <magnus@hagander.net> writes:

On Wed, Jan 2, 2013 at 4:14 AM, Andrew Dunstan <andrew@dunslane.net> wrote:

This seems to have broken plperl builds on Windows.

I'm not really sure what the best thing is to do here. We can either
use the fact that we know that uid_t and gid_t are "int" on all our
platforms, more or less, and change the tar api to use uid_t. Or put
the tar functions in their own header and make sure we don't include
that one anywhere that we include the perl headers.

Why are these very tar-specific functions being declared in such a
globally visible spot as port.h? That seems like a bad idea on its
face. IMO stuff in port.h ought to be about as globally applicable
as, say, malloc().

If there's actually a reason for that sort of namespace abuse, then
change their parameters to int --- IIRC, the tar format can't support
UIDs wider than 32 bits anyway.

regards, tom lane

--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

On Wed, Jan 2, 2013 at 5:36 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Magnus Hagander <magnus@hagander.net> writes:

On Wed, Jan 2, 2013 at 4:14 AM, Andrew Dunstan <andrew@dunslane.net> wrote:

This seems to have broken plperl builds on Windows.

I'm not really sure what the best thing is to do here. We can either
use the fact that we know that uid_t and gid_t are "int" on all our
platforms, more or less, and change the tar api to use uid_t. Or put
the tar functions in their own header and make sure we don't include
that one anywhere that we include the perl headers.

Why are these very tar-specific functions being declared in such a
globally visible spot as port.h? That seems like a bad idea on its
face. IMO stuff in port.h ought to be about as globally applicable
as, say, malloc().

It's where we put most of the things from src/port in.

I take it you suggest moving it to a special say include/pgtar.h file?

If there's actually a reason for that sort of namespace abuse, then
change their parameters to int --- IIRC, the tar format can't support
UIDs wider than 32 bits anyway.

Andrew suggested #ifndef'ing the declarations the same way that the
typedefs for uid_t and gid_t are done as another optin. I don't really
find either one of them non-kludgy :)

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Magnus Hagander <magnus@hagander.net> writes:

On Wed, Jan 2, 2013 at 5:36 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Why are these very tar-specific functions being declared in such a
globally visible spot as port.h? That seems like a bad idea on its
face. IMO stuff in port.h ought to be about as globally applicable
as, say, malloc().

It's where we put most of the things from src/port in.

Might be time to revisit that, or perhaps better reconsider what we're
putting in src/port/. The idea that anything that we want in both
frontend and backend is a porting issue seems a bit busted in itself.

I take it you suggest moving it to a special say include/pgtar.h file?

Works for me.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Wed, Jan 2, 2013 at 5:44 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Magnus Hagander <magnus@hagander.net> writes:

On Wed, Jan 2, 2013 at 5:36 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Why are these very tar-specific functions being declared in such a
globally visible spot as port.h? That seems like a bad idea on its
face. IMO stuff in port.h ought to be about as globally applicable
as, say, malloc().

It's where we put most of the things from src/port in.

Might be time to revisit that, or perhaps better reconsider what we're
putting in src/port/. The idea that anything that we want in both
frontend and backend is a porting issue seems a bit busted in itself.

Yeah, true - but there's certainly other parts that think the same, so
that's a separate issue.

I take it you suggest moving it to a special say include/pgtar.h file?

Works for me.

Applied. Should hopefully work once Alvaro gets the buildfarm back to green.

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers