pg_xlogdump -p option correction

On Tue, Dec 27, 2016 at 6:56 AM, Bruce Momjian <bruce@momjian.us> wrote:

I have found that pg_xlogdump looks for WAL files in the in the current
directory and the pg_wal subdirectory of both the current directory and
the PGDATA directory. Doc patch attached.

The whole behavior is listed in pg_xlogdump.c here:
/*
* Try to find the file in several places:
* if directory == NULL:
* fname
* XLOGDIR / fname
* $PGDATA / XLOGDIR / fname
* else
* directory / fname
* directory / XLOGDIR / fname
*
* return a read only fd
*/
static int
fuzzy_open_file(const char *directory, const char *fname)

Updating the docs the way your patch does does not look enough IMO:
1) pg_xlogdump --help should mention PGDATA as well for the default
behavior, and should make clear the default and when -p is defined:
-p, --path=PATH directory in which to find log segment files
(default: ./pg_wal)
2) The documentation does not mention that if a directory is defined
pg_xlogdump will try to look as well at defined_dir/pg_wal.
--
Michael

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

On Tue, Dec 27, 2016 at 01:28:17PM +0900, Michael Paquier wrote:

On Tue, Dec 27, 2016 at 6:56 AM, Bruce Momjian <bruce@momjian.us> wrote:

I have found that pg_xlogdump looks for WAL files in the in the current
directory and the pg_wal subdirectory of both the current directory and
the PGDATA directory. Doc patch attached.

The whole behavior is listed in pg_xlogdump.c here:
/*
* Try to find the file in several places:
* if directory == NULL:
* fname
* XLOGDIR / fname
* $PGDATA / XLOGDIR / fname
* else
* directory / fname
* directory / XLOGDIR / fname
*
* return a read only fd
*/
static int
fuzzy_open_file(const char *directory, const char *fname)

Updating the docs the way your patch does does not look enough IMO:

OK.

1) pg_xlogdump --help should mention PGDATA as well for the default
behavior, and should make clear the default and when -p is defined:
-p, --path=PATH directory in which to find log segment files
(default: ./pg_wal)

Ah, good point. The new output is:

-p, --path=PATH directory in which to find log segment files
(default: current directory, ./pg_wal, $PGDATA/pg_wal)

2) The documentation does not mention that if a directory is defined
pg_xlogdump will try to look as well at defined_dir/pg_wal.

Uh, I think it does in the first sentence:

Directory in which to find log segment files.

Updated patch attached.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +

On Sat, Jan 7, 2017 at 10:02 AM, Bruce Momjian <bruce@momjian.us> wrote:

1) pg_xlogdump --help should mention PGDATA as well for the default
behavior, and should make clear the default and when -p is defined:
-p, --path=PATH directory in which to find log segment files
(default: ./pg_wal)

Ah, good point. The new output is:

-p, --path=PATH directory in which to find log segment files
(default: current directory, ./pg_wal, $PGDATA/pg_wal)

Looks good, thanks.

2) The documentation does not mention that if a directory is defined
pg_xlogdump will try to look as well at defined_dir/pg_wal.

Uh, I think it does in the first sentence:

Directory in which to find log segment files.

I am reading that as "only the current directory", not "the current
directory, then current directory + pg_wal". Do you think that this
sentence implies that the check routine looks as well at
current_dir/pg_wal?

Updated patch attached.

+        subdirectory of both the current directory and the
+        <envar>PGDATA</envar> directory.
This could say "of both the current directory and *then* the PGDATA
directory" to outline the order of the actions taken by the check
routine. Just my 2c on the matter.
-- 
Michael

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

On Sat, Jan 7, 2017 at 09:50:32PM +0900, Michael Paquier wrote:

2) The documentation does not mention that if a directory is defined
pg_xlogdump will try to look as well at defined_dir/pg_wal.

Uh, I think it does in the first sentence:

Directory in which to find log segment files.

I am reading that as "only the current directory", not "the current
directory, then current directory + pg_wal". Do you think that this
sentence implies that the check routine looks as well at
current_dir/pg_wal?

Updated patch attached.

+        subdirectory of both the current directory and the
+        <envar>PGDATA</envar> directory.
This could say "of both the current directory and *then* the PGDATA
directory" to outline the order of the actions taken by the check
routine. Just my 2c on the matter.

Ah, I see your point. I ended up rewording the text to be more explicit
about the supplied argument and "searching". Updated patch attached.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +

On Sun, Jan 8, 2017 at 1:20 AM, Bruce Momjian <bruce@momjian.us> wrote:

On Sat, Jan 7, 2017 at 09:50:32PM +0900, Michael Paquier wrote:

2) The documentation does not mention that if a directory is defined
pg_xlogdump will try to look as well at defined_dir/pg_wal.

Uh, I think it does in the first sentence:

Directory in which to find log segment files.

I am reading that as "only the current directory", not "the current
directory, then current directory + pg_wal". Do you think that this
sentence implies that the check routine looks as well at
current_dir/pg_wal?

Updated patch attached.

+        subdirectory of both the current directory and the
+        <envar>PGDATA</envar> directory.
This could say "of both the current directory and *then* the PGDATA
directory" to outline the order of the actions taken by the check
routine. Just my 2c on the matter.

Ah, I see your point. I ended up rewording the text to be more explicit
about the supplied argument and "searching". Updated patch attached.

-        Directory in which to find log segment files.  The default is to search
-        for them in the <literal>pg_wal</literal> subdirectory of the current
-        directory.
+        Specifies a directory in which to find log segment files.
+        In addition, searches are performed in the current directory,
+        and the <literal>pg_wal</literal> subdirectory of both the current
+        directory and the <envar>PGDATA</envar> directory.
Hm. This still misses the point that the lookup at PGDATA is done only
if no directory is defined. What do you think about the patch attached
that lists the five possible patterns: 2 for the case where a
directory is defined, and 3 for the default case.
-- 
Michael

On Sun, Jan 8, 2017 at 03:27:59PM +0900, Michael Paquier wrote:

+        subdirectory of both the current directory and the
+        <envar>PGDATA</envar> directory.
This could say "of both the current directory and *then* the PGDATA
directory" to outline the order of the actions taken by the check
routine. Just my 2c on the matter.

Ah, I see your point. I ended up rewording the text to be more explicit
about the supplied argument and "searching". Updated patch attached.

-        Directory in which to find log segment files.  The default is to search
-        for them in the <literal>pg_wal</literal> subdirectory of the current
-        directory.
+        Specifies a directory in which to find log segment files.
+        In addition, searches are performed in the current directory,
+        and the <literal>pg_wal</literal> subdirectory of both the current
+        directory and the <envar>PGDATA</envar> directory.
Hm. This still misses the point that the lookup at PGDATA is done only
if no directory is defined. What do you think about the patch attached
that lists the five possible patterns: 2 for the case where a
directory is defined, and 3 for the default case.

Understood. Here is an updated patch which incorporates your
suggestions.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +

On Tue, Jan 10, 2017 at 4:32 AM, Bruce Momjian <bruce@momjian.us> wrote:

On Sun, Jan 8, 2017 at 03:27:59PM +0900, Michael Paquier wrote:

+        subdirectory of both the current directory and the
+        <envar>PGDATA</envar> directory.
This could say "of both the current directory and *then* the PGDATA
directory" to outline the order of the actions taken by the check
routine. Just my 2c on the matter.

Ah, I see your point. I ended up rewording the text to be more explicit
about the supplied argument and "searching". Updated patch attached.

-        Directory in which to find log segment files.  The default is to search
-        for them in the <literal>pg_wal</literal> subdirectory of the current
-        directory.
+        Specifies a directory in which to find log segment files.
+        In addition, searches are performed in the current directory,
+        and the <literal>pg_wal</literal> subdirectory of both the current
+        directory and the <envar>PGDATA</envar> directory.
Hm. This still misses the point that the lookup at PGDATA is done only
if no directory is defined. What do you think about the patch attached
that lists the five possible patterns: 2 for the case where a
directory is defined, and 3 for the default case.

Understood. Here is an updated patch which incorporates your
suggestions.

Thanks. This looks good to me.
--
Michael

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

On Tue, Jan 10, 2017 at 07:22:17AM +0900, Michael Paquier wrote:

On Tue, Jan 10, 2017 at 4:32 AM, Bruce Momjian <bruce@momjian.us> wrote:

On Sun, Jan 8, 2017 at 03:27:59PM +0900, Michael Paquier wrote:

+        subdirectory of both the current directory and the
+        <envar>PGDATA</envar> directory.
This could say "of both the current directory and *then* the PGDATA
directory" to outline the order of the actions taken by the check
routine. Just my 2c on the matter.

Ah, I see your point. I ended up rewording the text to be more explicit
about the supplied argument and "searching". Updated patch attached.

-        Directory in which to find log segment files.  The default is to search
-        for them in the <literal>pg_wal</literal> subdirectory of the current
-        directory.
+        Specifies a directory in which to find log segment files.
+        In addition, searches are performed in the current directory,
+        and the <literal>pg_wal</literal> subdirectory of both the current
+        directory and the <envar>PGDATA</envar> directory.
Hm. This still misses the point that the lookup at PGDATA is done only
if no directory is defined. What do you think about the patch attached
that lists the five possible patterns: 2 for the case where a
directory is defined, and 3 for the default case.

Understood. Here is an updated patch which incorporates your
suggestions.

Thanks. This looks good to me.

Slightly modified version applied and backpatched to 9.6 (use pg_xlog).
Thanks for your help.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +

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