Обсуждение: Add option --drop-cascade for pg_dump/restore

Overall the patch looks good, but I did notice a few small things:

1. In pg_dumpall.c, the section  /* Add long options to the pg_dump argument list */, we are now 
passing along the --drop-cascade option. However, --clean is not passed in, so 
any call to pg_dumpall using --drop-cascade fails a the pg_dump step. You'll note 
that --if-exists it not passed along either; because we are dropping the whole database, we don't 
need to have pg_dump worry about dropping objects at all. So I think that 
--drop-cascade should NOT be passed along from pg_dumpall to pg_dump.

2. I'm not even sure if --drop-cascade makes sense for pg_dumpall, as you cannot cascade global things like databases
androles.
 

3. In the file pg_backup_archiver.c, the patch does a 
stmtEnd = strstr(mark + strlen(buffer), ";");" and then spits 
out things "past" the semicolon as the final %s in the appendPQExpBuffer line. 
I'm not clear why: are we expecting more things to appear after the semi-colon? 
Why not just append a "\n" manually as part of the previous %s?

Cheers,
Greg

The new status of this patch is: Waiting on Author

Hi,

I agree that —drop-cascade does not make sense for pg_dumpall, so I removed them.

> are we expecting more things to appear after the semi-colon?

No, I was just trying to “reuse” original statement as much as possible. Append “\n” manually should also do the job,
andI’ve updated the patch as you suggests. 



> 2021年5月29日 上午2:39，Greg Sabino Mullane <htamfids@gmail.com> 写道：
>
> Overall the patch looks good, but I did notice a few small things:
>
> 1. In pg_dumpall.c, the section  /* Add long options to the pg_dump argument list */, we are now
> passing along the --drop-cascade option. However, --clean is not passed in, so
> any call to pg_dumpall using --drop-cascade fails a the pg_dump step. You'll note
> that --if-exists it not passed along either; because we are dropping the whole database, we don't
> need to have pg_dump worry about dropping objects at all. So I think that
> --drop-cascade should NOT be passed along from pg_dumpall to pg_dump.
>
> 2. I'm not even sure if --drop-cascade makes sense for pg_dumpall, as you cannot cascade global things like databases
androles. 
>
> 3. In the file pg_backup_archiver.c, the patch does a
> stmtEnd = strstr(mark + strlen(buffer), ";");" and then spits
> out things "past" the semicolon as the final %s in the appendPQExpBuffer line.
> I'm not clear why: are we expecting more things to appear after the semi-colon?
> Why not just append a "\n" manually as part of the previous %s?
>
> Cheers,
> Greg
>
> The new status of this patch is: Waiting on Author

On Fri, Jul 2, 2021 at 12:11 PM Haotian Wu <whtsky@gmail.com> wrote:
>
> Hi,
>
> I agree that —drop-cascade does not make sense for pg_dumpall, so I removed them.
>
> > are we expecting more things to appear after the semi-colon?
>
> No, I was just trying to “reuse” original statement as much as possible. Append “\n” manually should also do the job,
andI’ve updated the patch as you suggests. 

1) This change is not required as it is not supported for pg_dumpall
+++ b/doc/src/sgml/ref/pg_dumpall.sgml
@@ -289,6 +289,16 @@ PostgreSQL documentation
       </listitem>
      </varlistentry>

+     <varlistentry>
+      <term><option>--drop-cascade</option></term>
+      <listitem>
+       <para>
+        Use <literal>CASCADE</literal> to drop database objects.
+        This option is not valid unless <option>--clean</option> is
also specified.
+       </para>
+      </listitem>
+     </varlistentry>
+

2) I felt pg_dump will include the cascade option for plain format and
pg_restore will include the cascade option from pg_restore for other
formats. If my understanding is correct, should we document this?

3) This change is not required

destroyPQExpBuffer(ftStmt);
                                                        pg_free(dropStmtOrig);
                                                }
+
                                        }

4) Is it possible to add a few tests for this?

Regards,
Vignesh

> 2) I felt pg_dump will include the cascade option for plain format and
> pg_restore will include the cascade option from pg_restore for other
> formats. If my understanding is correct, should we document this?

I may not understand it correctly, are you saying
pg_dump will include the cascade option only for plain format, or
pg_dump will enable the cascade option for plain by default?

> 4) Is it possible to add a few tests for this?

Looks like tests should be added to
`src/bin/pg_dump/t/002_pg_dump.pl`, I'll try to add some.

vignesh C <vignesh21@gmail.com> 于2021年7月13日周二 下午10:23写道：
>
> On Fri, Jul 2, 2021 at 12:11 PM Haotian Wu <whtsky@gmail.com> wrote:
> >
> > Hi,
> >
> > I agree that —drop-cascade does not make sense for pg_dumpall, so I removed them.
> >
> > > are we expecting more things to appear after the semi-colon?
> >
> > No, I was just trying to “reuse” original statement as much as possible. Append “\n” manually should also do the
job,and I’ve updated the patch as you suggests. 
>
> 1) This change is not required as it is not supported for pg_dumpall
> +++ b/doc/src/sgml/ref/pg_dumpall.sgml
> @@ -289,6 +289,16 @@ PostgreSQL documentation
>        </listitem>
>       </varlistentry>
>
> +     <varlistentry>
> +      <term><option>--drop-cascade</option></term>
> +      <listitem>
> +       <para>
> +        Use <literal>CASCADE</literal> to drop database objects.
> +        This option is not valid unless <option>--clean</option> is
> also specified.
> +       </para>
> +      </listitem>
> +     </varlistentry>
> +
>
> 2) I felt pg_dump will include the cascade option for plain format and
> pg_restore will include the cascade option from pg_restore for other
> formats. If my understanding is correct, should we document this?
>
> 3) This change is not required
>
> destroyPQExpBuffer(ftStmt);
>                                                         pg_free(dropStmtOrig);
>                                                 }
> +
>                                         }
>
> 4) Is it possible to add a few tests for this?
>
> Regards,
> Vignesh

On Tue, Jul 13, 2021 at 9:16 PM Wu Haotian <whtsky@gmail.com> wrote:
>
> > 2) I felt pg_dump will include the cascade option for plain format and
> > pg_restore will include the cascade option from pg_restore for other
> > formats. If my understanding is correct, should we document this?
>
> I may not understand it correctly, are you saying
> pg_dump will include the cascade option only for plain format, or
> pg_dump will enable the cascade option for plain by default?

pg_dump support plain, custom, tar and directory format, I think,
cascade option will be added by pg_dump only for plain format and for
the other format pg_restore will include the cascade option. Should we
document this somewhere?

> > 4) Is it possible to add a few tests for this?
>
> Looks like tests should be added to
> `src/bin/pg_dump/t/002_pg_dump.pl`, I'll try to add some.

Yes, that should be the right place.

Regards,
Vignesh

vignesh C <vignesh21@gmail.com> writes:
> On Tue, Jul 13, 2021 at 9:16 PM Wu Haotian <whtsky@gmail.com> wrote:
>> I may not understand it correctly, are you saying
>> pg_dump will include the cascade option only for plain format, or
>> pg_dump will enable the cascade option for plain by default?

> pg_dump support plain, custom, tar and directory format, I think,
> cascade option will be added by pg_dump only for plain format and for
> the other format pg_restore will include the cascade option. Should we
> document this somewhere?

That would require pg_restore to try to edit the DROP commands during
restore, which sounds horribly fragile.  I'm inclined to think that
supporting this option only during initial dump is safer.

            regards, tom lane

On 7/16/21 9:40 AM, Tom Lane wrote:
> vignesh C <vignesh21@gmail.com> writes:
>> On Tue, Jul 13, 2021 at 9:16 PM Wu Haotian <whtsky@gmail.com> wrote:
>>> I may not understand it correctly, are you saying
>>> pg_dump will include the cascade option only for plain format, or
>>> pg_dump will enable the cascade option for plain by default?
>> pg_dump support plain, custom, tar and directory format, I think,
>> cascade option will be added by pg_dump only for plain format and for
>> the other format pg_restore will include the cascade option. Should we
>> document this somewhere?
> That would require pg_restore to try to edit the DROP commands during
> restore, which sounds horribly fragile.  I'm inclined to think that
> supporting this option only during initial dump is safer.
>
>             



Maybe, but that would push back the time when you would need to decide
you needed this quite a lot. We could also have pg_dump stash a copy of
the CASCADE variant in the TOC that could be used by pg_restore if
required. I'm not sure if it's worth the trouble and extra space though.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On Tue, Aug 10, 2021 at 10:57 PM Greg Sabino Mullane <htamfids@gmail.com> wrote:
>
> On Fri, Jul 16, 2021 at 9:40 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>
>> That would require pg_restore to try to edit the DROP commands during
>> restore, which sounds horribly fragile.  I'm inclined to think that
>> supporting this option only during initial dump is safer.
>
>
> Safer, but not nearly as useful. Maybe see what the OP (Wu Haotian) can come up with as a first implementation?
>
> Cheers,
> Greg
>

pg_restore already tries to edit the DROP commands during restore in
order to support `--if-exists`.

> supporting this option only during initial dump is safer.

pg_dump & pg_restores use the same function to inject `IF EXISTS` (
and `DROP .. CASCADE` in this patch`).
Supporting this option only during pg_dump may not make it safer, as
the logic is the same.

Hi,

I've updated the patch to remove unnecessary changes and added tests.

On Fri, Jul 16, 2021 at 9:09 PM vignesh C <vignesh21@gmail.com> wrote:
> pg_dump support plain, custom, tar and directory format, I think,
> cascade option will be added by pg_dump only for plain format and for
> the other format pg_restore will include the cascade option. Should we
> document this somewhere?

Yes, cascade option relies on `--clean` which only works for plain
format in pg_dump.
Maybe we can add checks like "option --clean requires plain text format"?
If so, should I start a new mail thread for this?

There are already documents for "--clean only works with plain text output",
so adding checks for --clean seems like a breaking change to me.

I've updated the docs to indicate --drop-cascade and --if-exists only
works with plain text output.

> On 16 Aug 2021, at 08:35, Wu Haotian <whtsky@gmail.com> wrote:
>
> There are already documents for "--clean only works with plain text output",
> so adding checks for --clean seems like a breaking change to me.
>
> I've updated the docs to indicate --drop-cascade and --if-exists only
> works with plain text output.

This patch fails to apply after recent changes to the pg_dump TAP tests.
Please submit a rebased version.

--
Daniel Gustafsson        https://vmware.com/

Hi,
here's the rebased patch.

Wu Haotian <whtsky@gmail.com> writes:
> here's the rebased patch.

Looks like it needs rebasing again, probably as a result of our recent
renaming of our Perl test modules.

FWIW, I'd strongly recommend that it's time to pull all that SQL code
hacking out of RestoreArchive and put it in its own function.

            regards, tom lane

> On 3 Nov 2021, at 20:03, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> Wu Haotian <whtsky@gmail.com> writes:
>> here's the rebased patch.
> 
> Looks like it needs rebasing again, probably as a result of our recent
> renaming of our Perl test modules.

As this patch hasn't been updated, I'm marking this entry Returned with
Feedback.  Please feel free to open a new entry when a rebased patch is
available.

> FWIW, I'd strongly recommend that it's time to pull all that SQL code
> hacking out of RestoreArchive and put it in its own function.

+1

--
Daniel Gustafsson        https://vmware.com/