Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*)

From: Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr>
To: Justin Pryzby <pryzby(at)telsasoft(dot)com>
Cc: "Bossart, Nathan" <bossartn(at)amazon(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Stephen Frost <sfrost(at)snowman(dot)net>, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, David Steele <david(at)pgmasters(dot)net>, pgsql-hackers(at)postgresql(dot)org, Thomas Munro <thomas(dot)munro(at)gmail(dot)com>, Michael Paquier <michael(at)paquier(dot)xyz>
Subject: Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*)
Date: 2022-03-12 09:13:21
Message-ID: alpine.DEB.2.22.394.2203121010400.3679190@pseudo
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers


Hello Justin,

Review about v34, up from v32 which I reviewed in January. v34 is an
updated version of v32, without the part about lstat at the end of the
series.

All 7 patches apply cleanly.

# part 01

One liner doc improvement about the directory flag.

OK.

# part 02

Add tests for various options on pg_ls_dir, and for pg_stat_file, which were not
exercised before. "make check" is ok.

OK.

# part 03

This patch adds a new pg_ls_dir_metadata. Internally, this is an extension of
pg_ls_dir_files function which is used by other pg_ls functions. Doc ok.

New tests are added which check that the result columns are configured as required,
including error cases.

"make check" is ok.

OK.

# part 04

Add a new "isdir" column to "pg_ls_tmpdir" output. This is a small behavioral
change.

I'm ok with that, however I must say that I'm still unsure why we would
not jump directly to a "type" char column. What is wrong with outputing
'd' or '-' instead of true or false? This way, the interface needs not
change if "lstat" is used later? ISTM that the interface issue should be
somehow independent of the implementation issue, and we should choose
directly the right/best interface?

Independently, the documentation may be clearer about what happens to
"isdir" when the file is a link? It may say that the behavior may change
in the future?

About homogeneity, I note that some people may be against adding "isdir"
to other ls functions. I must say that I cannot see a good argument not to
do it, and that I hate dealing with systems which are not homogeneous
because it creates surprises and some loss of time.

"make check" ok.

OK.

# part 05

Add isdir to other ls functions. Doc is updated.

Same as above, I'd prefer a char instead of a bool, as it is more extendable and
future-proof.

OK.

# part 06

This part extends and adds a test for pg_ls_logdir.
"make check" is ok.

OK.

# part 07

This part extends pg_stat_file with more date informations.

"make check" is ok.

OK.

# doc

Overall doc generation is OK.

--
Fabien.

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Dilip Kumar 2022-03-12 09:49:32 Re: [Proposal] Fully WAL logged CREATE DATABASE - No Checkpoints
Previous Message vignesh C 2022-03-12 07:59:06 Re: Logical replication - schema change not invalidating the relation cache