Thread: pg_upgrade generated files in subdir follow-up
Commit 38bfae36526 moved the .txt files pg_upgrade generates to a separate subdir, but there are a few left which are written to cwd. The thread resulting in that patch doesn't discuss these files specifically so it seems they are just an oversight. Unless I'm missing something. Should something the attached be applied to ensure all generated files are placed in the subdirectory? -- Daniel Gustafsson https://vmware.com/
Attachment
Daniel Gustafsson <daniel@yesql.se> writes:
> Commit 38bfae36526 moved the .txt files pg_upgrade generates to a separate
> subdir, but there are a few left which are written to cwd. The thread
> resulting in that patch doesn't discuss these files specifically so it seems
> they are just an oversight. Unless I'm missing something.
> Should something the attached be applied to ensure all generated files are
> placed in the subdirectory?
It certainly looks inconsistent ATM. I wondered if maybe the plan was to
put routine output into the log directory but problem-reporting files
into cwd --- but that isn't what's happening now.
As long as we report the path to where the file is, I don't see a reason
not to put problem-reporting files in the subdir too.
regards, tom lane
> On 31 Aug 2022, at 15:59, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Daniel Gustafsson <daniel@yesql.se> writes:
>> Commit 38bfae36526 moved the .txt files pg_upgrade generates to a separate
>> subdir, but there are a few left which are written to cwd. The thread
>> resulting in that patch doesn't discuss these files specifically so it seems
>> they are just an oversight. Unless I'm missing something.
>
>> Should something the attached be applied to ensure all generated files are
>> placed in the subdirectory?
>
> It certainly looks inconsistent ATM. I wondered if maybe the plan was to
> put routine output into the log directory but problem-reporting files
> into cwd --- but that isn't what's happening now.
Right, check_proper_datallowconn and check_for_isn_and_int8_passing_mismatch
and a few other check functions already place error reporting in the subdir.
> As long as we report the path to where the file is, I don't see a reason
> not to put problem-reporting files in the subdir too.
Agreed. The documentation states:
"pg_upgrade creates various working files, such as schema dumps, stored
within pg_upgrade_output.d in the directory of the new cluster. Each
run creates a new subdirectory named with a timestamp formatted as per
ISO 8601 (%Y%m%dT%H%M%S), where all the generated files are stored."
The delete_old_cluster and reindex_hash scripts are still placed in CWD, which
isn't changed by this patch, as that seems correct (and might break scripts if
we move them). Maybe we should amend the docs to mention that scripts aren't
generated in the subdir?
--
Daniel Gustafsson https://vmware.com/
On Wed, Aug 31, 2022 at 04:41:24PM +0200, Daniel Gustafsson wrote: > > On 31 Aug 2022, at 15:59, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > > > Daniel Gustafsson <daniel@yesql.se> writes: > >> Commit 38bfae36526 moved the .txt files pg_upgrade generates to a separate > >> subdir, but there are a few left which are written to cwd. The thread > >> resulting in that patch doesn't discuss these files specifically so it seems > >> they are just an oversight. Unless I'm missing something. > > > >> Should something the attached be applied to ensure all generated files are > >> placed in the subdirectory? > > > > It certainly looks inconsistent ATM. I wondered if maybe the plan was to > > put routine output into the log directory but problem-reporting files > > into cwd --- but that isn't what's happening now. The script files are intended to stay where they are, and the error files are intended to move under the subdir, to allow for their easy removal, per Tom's request. > Right, check_proper_datallowconn and check_for_isn_and_int8_passing_mismatch > and a few other check functions already place error reporting in the subdir. It looks like I may have grepped for fprintf or similar, and missed checking output_path. I updated your patach to put the logic inside check_for_data_types_usage(), which is shorter, and seems to simplify doing what's intended into the future. -- Justin
Attachment
On Fri, Sep 09, 2022 at 11:07:41AM -0500, Justin Pryzby wrote: > On Wed, Aug 31, 2022 at 04:41:24PM +0200, Daniel Gustafsson wrote: > > > On 31 Aug 2022, at 15:59, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > > > > > Daniel Gustafsson <daniel@yesql.se> writes: > > >> Commit 38bfae36526 moved the .txt files pg_upgrade generates to a separate > > >> subdir, but there are a few left which are written to cwd. The thread > > >> resulting in that patch doesn't discuss these files specifically so it seems > > >> they are just an oversight. Unless I'm missing something. > > > > > >> Should something the attached be applied to ensure all generated files are > > >> placed in the subdirectory? > > > > > > It certainly looks inconsistent ATM. I wondered if maybe the plan was to > > > put routine output into the log directory but problem-reporting files > > > into cwd --- but that isn't what's happening now. > > The script files are intended to stay where they are, and the error > files are intended to move under the subdir, to allow for their easy > removal, per Tom's request. Right. The .txt files reporting that something went wrong should be in the basedir, like loadable_libraries.txt, as these are not really internal logs but provide information about a failure state. I have double-checked the whole code of pg_upgrade, and I am not seeing another area to fix, so 0001 looks fine to me. This one is on me, so I guess that I'd like to take care of it myself. > It looks like I may have grepped for fprintf or similar, and missed > checking output_path. > > I updated your patach to put the logic inside > check_for_data_types_usage(), which is shorter, and seems to simplify > doing what's intended into the future. 0002 makes the code more complicated IMO, as we still need to report the location of the file in the logs. So I would leave things to what's proposed in 0001. -- Michael
Attachment
> On 12 Sep 2022, at 09:33, Michael Paquier <michael@paquier.xyz> wrote: > I have double-checked the whole code of pg_upgrade, and I am not seeing another > area to fix, so 0001 looks fine to me. This one is on me, so I guess that I'd > like to take care of it myself. Sure, go for it. -- Daniel Gustafsson https://vmware.com/
On Mon, Sep 12, 2022 at 09:51:32AM +0200, Daniel Gustafsson wrote: > Sure, go for it. Thanks. Done, then, after an extra look. -- Michael