Expending the use of xlog_internal.h's macros

On Wed, Jun 10, 2015 at 2:41 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:

Hi all,

While looking at the code of pg_archivecleanup.c, I noticed that there
is some code present to detect if a given string has the format of a
WAL segment file name or of a backup file.
The recent commit 179cdd09 has introduced in xlog_internal.h a set of
macros to facilitate checks of pg_xlog's name format:
IsPartialXLogFileName(), IsXLogFileName() and IsTLHistoryFileName().

And by looking at the code, there are some utilities where we could
make use of that, like pg_resetxlog, pg_archivecleanup and pg_standby.

Attached is a small refactoring patch doing so for HEAD.

Thanks for the patch!

I updated the patch as follows. Patch attached.

+#define XLogFileNameExtended(fname, tli, log, seg)

Move this macro to xlog_internal.h because it's used both in
pg_standby and pg_archivecleanup. There seems no need to
define it independently.

-#define MAXFNAMELEN        64
+#define MAXFNAMELEN                64

Revert this unnecessary change.

+/* Length of XLog file name */
+#define XLOG_DATA_FNAME_LEN 24

Shorten the name of this macro variable, to XLOG_FNAME_LEN,
for more code readability.

Comments?

Regards,

--
Fujii Masao

--
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, Jul 1, 2015 at 8:16 PM, Fujii Masao <masao.fujii@gmail.com> wrote:

On Wed, Jun 10, 2015 at 2:41 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:

Hi all,

While looking at the code of pg_archivecleanup.c, I noticed that there
is some code present to detect if a given string has the format of a
WAL segment file name or of a backup file.
The recent commit 179cdd09 has introduced in xlog_internal.h a set of
macros to facilitate checks of pg_xlog's name format:
IsPartialXLogFileName(), IsXLogFileName() and IsTLHistoryFileName().

And by looking at the code, there are some utilities where we could
make use of that, like pg_resetxlog, pg_archivecleanup and pg_standby.

Attached is a small refactoring patch doing so for HEAD.

Thanks for the patch!

I updated the patch as follows. Patch attached.

+#define XLogFileNameExtended(fname, tli, log, seg)

Move this macro to xlog_internal.h because it's used both in
pg_standby and pg_archivecleanup. There seems no need to
define it independently.

-#define MAXFNAMELEN        64
+#define MAXFNAMELEN                64

Revert this unnecessary change.

+/* Length of XLog file name */
+#define XLOG_DATA_FNAME_LEN 24

Shorten the name of this macro variable, to XLOG_FNAME_LEN,
for more code readability.

Comments?

Regards,

--
Fujii Masao

Patch attached.

Regards,

--
Fujii Masao

On Wed, Jul 1, 2015 at 8:18 PM, Fujii Masao wrote:

On Wed, Jul 1, 2015 at 8:16 PM, Fujii Masao wrote:

I updated the patch as follows. Patch attached.

+#define XLogFileNameExtended(fname, tli, log, seg)

Move this macro to xlog_internal.h because it's used both in
pg_standby and pg_archivecleanup. There seems no need to
define it independently.

OK for me.

-#define MAXFNAMELEN        64
+#define MAXFNAMELEN                64

Revert this unnecessary change.

Yes, thanks.

+/* Length of XLog file name */
+#define XLOG_DATA_FNAME_LEN 24

Shorten the name of this macro variable, to XLOG_FNAME_LEN,
for more code readability.

Thanks. You have more talent for naming than I do.

Comments?

Just reading it again, I think that XLogFileNameById should use
MAXFNAMELEN, and that XLogFileName should call directly
XLogFileNameById as both are doing the same operation like in the
attached. It seems also safer to use MAXFNAMELEN instead of MAXPGPATH
for exclusiveCleanupFileName in pg_standby.c and pg_archivecleanup.c.
--
Michael

On Wed, Jul 1, 2015 at 8:58 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:

On Wed, Jul 1, 2015 at 8:18 PM, Fujii Masao wrote:

On Wed, Jul 1, 2015 at 8:16 PM, Fujii Masao wrote:

I updated the patch as follows. Patch attached.

+#define XLogFileNameExtended(fname, tli, log, seg)

Move this macro to xlog_internal.h because it's used both in
pg_standby and pg_archivecleanup. There seems no need to
define it independently.

OK for me.

-#define MAXFNAMELEN        64
+#define MAXFNAMELEN                64

Revert this unnecessary change.

Yes, thanks.

+/* Length of XLog file name */
+#define XLOG_DATA_FNAME_LEN 24

Shorten the name of this macro variable, to XLOG_FNAME_LEN,
for more code readability.

Thanks. You have more talent for naming than I do.

Comments?

Just reading it again, I think that XLogFileNameById should use
MAXFNAMELEN, and that XLogFileName should call directly
XLogFileNameById as both are doing the same operation like in the
attached.

We can refactor the code that way, but which looks a bit messy
at least to me. The original coding looks simpler and easier-readable,
so I'd like to adopt the original one here.

It seems also safer to use MAXFNAMELEN instead of MAXPGPATH
for exclusiveCleanupFileName in pg_standby.c and pg_archivecleanup.c.

Yep.

Regards,

--
Fujii Masao

--
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, Jul 1, 2015 at 9:53 PM, Fujii Masao <masao.fujii@gmail.com> wrote:

On Wed, Jul 1, 2015 at 8:58 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:

On Wed, Jul 1, 2015 at 8:18 PM, Fujii Masao wrote:

On Wed, Jul 1, 2015 at 8:16 PM, Fujii Masao wrote:

I updated the patch as follows. Patch attached.

+#define XLogFileNameExtended(fname, tli, log, seg)

Move this macro to xlog_internal.h because it's used both in
pg_standby and pg_archivecleanup. There seems no need to
define it independently.

OK for me.

-#define MAXFNAMELEN        64
+#define MAXFNAMELEN                64

Revert this unnecessary change.

Yes, thanks.

+/* Length of XLog file name */
+#define XLOG_DATA_FNAME_LEN 24

Shorten the name of this macro variable, to XLOG_FNAME_LEN,
for more code readability.

Thanks. You have more talent for naming than I do.

Comments?

Just reading it again, I think that XLogFileNameById should use
MAXFNAMELEN, and that XLogFileName should call directly
XLogFileNameById as both are doing the same operation like in the
attached.

We can refactor the code that way, but which looks a bit messy
at least to me. The original coding looks simpler and easier-readable,
so I'd like to adopt the original one here.

It seems also safer to use MAXFNAMELEN instead of MAXPGPATH
for exclusiveCleanupFileName in pg_standby.c and pg_archivecleanup.c.

Yep.

Pushed. Thanks!

Regards,

--
Fujii Masao

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

On Thu, Jul 2, 2015 at 10:37 AM, Fujii Masao wrote:

On Wed, Jul 1, 2015 at 9:53 PM, Fujii Masao wrote:

+/* Length of XLog file name */
+#define XLOG_DATA_FNAME_LEN 24

Shorten the name of this macro variable, to XLOG_FNAME_LEN,
for more code readability.

Thanks. You have more talent for naming than I do.

Comments?

Just reading it again, I think that XLogFileNameById should use
MAXFNAMELEN, and that XLogFileName should call directly
XLogFileNameById as both are doing the same operation like in the
attached.

We can refactor the code that way, but which looks a bit messy
at least to me. The original coding looks simpler and easier-readable,
so I'd like to adopt the original one here.

It seems also safer to use MAXFNAMELEN instead of MAXPGPATH
for exclusiveCleanupFileName in pg_standby.c and pg_archivecleanup.c.

Yep.

Pushed. Thanks!

Thanks! I was going to send a patch with what you wanted but you just
beat me at it.
--
Michael

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