Initdb failure

On Thu, 25 Jul 2019 at 07:39, vignesh C <vignesh21@gmail.com> wrote:

Hi,

Initdb fails when following path is provided as input:
datasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafdds/datasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafdds/datasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafdds/datasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafdds/datasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafdds/datasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafdds/

Also the cleanup also tends to fail in the cleanup path.

Could be something to do with path handling.

This is because the value of MAXPGPATH is 1024 and the path you are
providing is more than that. Hence, when it is trying to read
PG_VERSION in ValidatePgVersion it is going to a wrong path with just
1024 characters.

I'm not sure if this is already known.

I am also not sure if it is known or intentional. On the other hand I
also don't know if it is practical to give such long names for
database directory anyway.

--
Regards,
Rafia Sabih

On Thu, Jul 25, 2019 at 4:52 PM Rafia Sabih <rafia.pghackers@gmail.com> wrote:

On Thu, 25 Jul 2019 at 07:39, vignesh C <vignesh21@gmail.com> wrote:

Hi,

Initdb fails when following path is provided as input:
datasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafdds/datasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafdds/datasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafdds/datasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafdds/datasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafdds/datasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafdds/

Also the cleanup also tends to fail in the cleanup path.

Could be something to do with path handling.

This is because the value of MAXPGPATH is 1024 and the path you are
providing is more than that. Hence, when it is trying to read
PG_VERSION in ValidatePgVersion it is going to a wrong path with just
1024 characters.

The error occurs at a very later point after performing the initial
work like creating directory. I'm thinking we should check this in
the beginning and throw the error message at the beginning and exit
cleanly.

I'm not sure if this is already known.

I am also not sure if it is known or intentional. On the other hand I
also don't know if it is practical to give such long names for
database directory anyway.

Usually they will not be using such long path, but this is one of the
odd scenarios.

Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com

On Thu, 25 Jul 2019 at 13:50, vignesh C <vignesh21@gmail.com> wrote:

On Thu, Jul 25, 2019 at 4:52 PM Rafia Sabih <rafia.pghackers@gmail.com> wrote:

On Thu, 25 Jul 2019 at 07:39, vignesh C <vignesh21@gmail.com> wrote:

Hi,

Initdb fails when following path is provided as input:
datasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafdds/datasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafdds/datasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafdds/datasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafdds/datasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafdds/datasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafdds/

Also the cleanup also tends to fail in the cleanup path.

Could be something to do with path handling.

This is because the value of MAXPGPATH is 1024 and the path you are
providing is more than that. Hence, when it is trying to read
PG_VERSION in ValidatePgVersion it is going to a wrong path with just
1024 characters.

The error occurs at a very later point after performing the initial
work like creating directory. I'm thinking we should check this in
the beginning and throw the error message at the beginning and exit
cleanly.

Now that you say this, it does make sense to atleast inform about the
correct error and that too earlier. Something like the attached patch
would make sense.

--
Regards,
Rafia Sabih

Rafia Sabih <rafia.pghackers@gmail.com> writes:

On Thu, 25 Jul 2019 at 13:50, vignesh C <vignesh21@gmail.com> wrote:

Initdb fails when following path is provided as input:
datasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafdds/datasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafdds/datasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafdds/datasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafdds/datasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafdds/datasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafdds/

Now that you say this, it does make sense to atleast inform about the
correct error and that too earlier. Something like the attached patch
would make sense.

I am not terribly excited about putting effort into this at all, because
I don't think that any actual user anywhere will ever get any benefit.
The proposed test case is just silly.

However, if we are going to put effort into it, it needs to be more than
this. First off, what is the actual failure point? (It's surely less
than MAXPGPATH, because we tend to append subdirectory/file names onto
whatever is given.) Second, what of absolute versus relative paths?
If converting the given path to absolute makes it exceed MAXPGPATH,
is that a problem?

regards, tom lane

On Thu, 25 Jul 2019 at 16:44, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Rafia Sabih <rafia.pghackers@gmail.com> writes:

On Thu, 25 Jul 2019 at 13:50, vignesh C <vignesh21@gmail.com> wrote:

Initdb fails when following path is provided as input:
datasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafdds/datasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafdds/datasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafdds/datasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafdds/datasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafdds/datasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafdds/

Now that you say this, it does make sense to atleast inform about the
correct error and that too earlier. Something like the attached patch
would make sense.

I am not terribly excited about putting effort into this at all, because
I don't think that any actual user anywhere will ever get any benefit.
The proposed test case is just silly.

That I totally agree upon!
But on the other hand emitting the right error message atleast would
be good for the sake of correctness if nothing else. But yes that
definitely should be weighed against what is the effort required for
this.

--
Regards,
Rafia Sabih

On Thu, Jul 25, 2019 at 8:39 PM Rafia Sabih <rafia.pghackers@gmail.com> wrote:

On Thu, 25 Jul 2019 at 16:44, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Rafia Sabih <rafia.pghackers@gmail.com> writes:

On Thu, 25 Jul 2019 at 13:50, vignesh C <vignesh21@gmail.com> wrote:

Initdb fails when following path is provided as input:
datasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafdds/datasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafdds/datasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafdds/datasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafdds/datasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafdds/datasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafdds/

Now that you say this, it does make sense to atleast inform about the
correct error and that too earlier. Something like the attached patch
would make sense.

I am not terribly excited about putting effort into this at all, because
I don't think that any actual user anywhere will ever get any benefit.
The proposed test case is just silly.

That I totally agree upon!
But on the other hand emitting the right error message atleast would
be good for the sake of correctness if nothing else. But yes that
definitely should be weighed against what is the effort required for
this.

Thanks Tom for your opinion.
Thanks Rafia for your thoughts and effort in making the patch.

I'm not sure if we are planning to fix this.
If we are planning to fix, one suggestion from my side we can
choose a safe length which would include the subdirectories
and file paths. I think one of these will be the longest:
base/database_oid/tables
pg_wal/archive_status/
pg_wal/archive_file

Fix can be something like:
MAXPGPATH - (LONGEST_PATH_FROM_ABOVE)

Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com

On 2019-07-25 17:09, Rafia Sabih wrote:

But on the other hand emitting the right error message atleast would
be good for the sake of correctness if nothing else. But yes that
definitely should be weighed against what is the effort required for
this.

I think if you want to make this more robust, get rid of the fixed-size
array, use dynamic allocation with PQExpBuffer, and let the operating
system complain if it doesn't like the directory name length.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Sat, Jul 27, 2019 at 2:22 AM Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:

I think if you want to make this more robust, get rid of the fixed-size
array, use dynamic allocation with PQExpBuffer, and let the operating
system complain if it doesn't like the directory name length.

Agreed, but I think we should just do nothing. To actually fix this
in general, we'd have to get rid of every instance of MAXPGPATH in the
source tree:

[rhaas pgsql]$ git grep MAXPGPATH | wc -l
611

If somebody feels motivated to spend that amount of effort improving
this, I will stand back and cheer from the sidelines, but that's gonna
be a LOT of work for a problem that, as Tom says, is probably not
really affecting very many people.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas@gmail.com> writes:

Agreed, but I think we should just do nothing. To actually fix this
in general, we'd have to get rid of every instance of MAXPGPATH in the
source tree:
[rhaas pgsql]$ git grep MAXPGPATH | wc -l
611

I don't think it'd really be necessary to go that far. One of the
reasons we chdir to the data directory at postmaster start is so that
(pretty nearly) all filenames that backends deal with are relative
pathnames of very predictable, short lengths. So a lot of those
MAXPGPATH uses are probably perfectly safe, indeed likely overkill.

However, identifying which ones are not safe would still take looking
at every use case, so I agree there'd be a lot of work here.

Would there be any value in teaching initdb to do something similar,
ie chdir to the parent of the target datadir location? Then the set
of places in initdb that have to deal with long pathnames would be
pretty well constrained.

On the whole though, I don't have a problem with the "do nothing"
answer. There's no security risk here, and no issue that seems
likely to arise in actual use cases rather than try-to-break-it
test scripts.

regards, tom lane

On Tue, Jul 30, 2019 at 03:30:12PM -0400, Tom Lane wrote:

On the whole though, I don't have a problem with the "do nothing"
answer. There's no security risk here, and no issue that seems
likely to arise in actual use cases rather than try-to-break-it
test scripts.

+1.
--
Michael