Re: Adding pg_dump flag for parallel export to pipes

On Thu, Jun 5, 2025 at 6:09 PM Nitin Motiani <nitinmotiani(at)google(dot)com> wrote:
>
> Hi,
>
> Apologies for the delay on this thread.
>
> On Mon, Apr 28, 2025 at 1:52 PM Nitin Motiani <nitinmotiani(at)google(dot)com> wrote:
> >
> > Thanks for the feedback, Thomas.
> >
> > > Do I understand correctly that
> > > the problem you encountered is in some other tests that you haven't
> > > attached yet? Could you post what you have so that others can see the
> > > problem and perhaps have a chance of helping?
> >
> > Yes, we didn't add the failed tests to the patch. We'll add those and
> > send new patches.
> >
>
> I'm attaching the patch files generated using git format-patch.
>
> 0001 has the pg_dump pipe support code.
> 0002 has the pg_restore pipe support.
> 0003 has a few basic validation tests for correct flag combinations.
> 0004 has the proposed documentation changes.
>
> The above 4 are the same as before.
>
> The 0005 patch is the new WIP patch file. This includes the tests
> which we have been trying to add but which are failing (although the
> same commands run fine manually).
>
> The tests in this patch are added to src/bin/pg_dump/t/002_pg_dump.pl.
> The original attempt was to have a test case with dump and restore
> commands using the new flag and run it in multiple scenarios. But
> since that was failing, for the ease of debugging I added a few
> standalone tests which just run a pg_dump with the pipe-command flag.
> In these tests, if the pipe-command is a simple command like 'cat' or
> 'gzip', the test passes. But if the pipe-command itself uses a pipe
> (either to a file or another command), the test fails.
>
> In the following test
>
> ['pg_dump', '-Fd', '-B', 'postgres', "--pipe-command=\"cat > $tempdir/%f\"",],]
>
> I get the below error.
>
> # 'sh: line 1: cat >
> /usr/local/google/home/nitinmotiani/postgresql/src/bin/pg_dump/tmp_check/tmp_test_XpFO/toc.dat:
> No such file or directory
>
> I can see that the temp directory tmp_test_XpFO exists. Even when I
> changed the test to use an absolute path to an existing directory, I
> got the same error. When I do manual testing with the same
> pipe-command, it works fine. That is why we think there is some issue
> with our environment setup for the tap test where it is not able to
> parse the command.
>
> I also ran the following loop (started just before starting the test
> run) to print the output of ps commands around 'cat >' to see what
> happens.
>
> for i in $(seq 1 10000); do ps --forest -ef | grep "cat >" -A 5 >>
> ~/ps_output.txt; done
>
> The printed results showed that the child process with the pipe
> command became defunct.
>
> nitinmo+ 3180211 3180160 5 17:05 pts/1 00:00:00 | |
> \_ /usr/local/google/home/nitinmotiani/postgresql/tmp_install/usr/local/pgsql/bin/pg_dump
> -Fd -B p ostgres --pipe-command="cat >
> /usr/local/google/home/nitinmotiani/postgresql/src/bin/pg_dump/definite_dumpdir/%f"
> nitinmo+ 3180215 3180211 0 17:05 pts/1 00:00:00 | |
> \_ [sh] <defunct>
>
> We are not sure how to handle this issue. Please let us know your thoughts.

The latest patch set is not applying on HEAD can you rebase the patch
set. And also there are many TODOs in the patch, if those TODOs are
just good to do and you are planning for future development better to
get rid of those. OTOH if some of those TODOs are mandatory to do
before we can commit the patch then are you planning to work on those
soon? I am planning to review this patch so are you planning to send
the rebased version with implementing the TODO which are required for
the first version.

--
Regards,
Dilip Kumar
Google