Обсуждение: pg_dump new feature: exporting functions only. Bad or good idea ?

On 3/17/21 6:00 PM, Lætitia Avrot wrote:
> Hey hackers,
> 
> I had this idea, that I raised and cherished like my baby to add a switch
> in `pg_dump` to allow exporting stored functions (and procedures) only.
> 
> However, when I finally got the time to look at it in detail, I found out
> there was no way to solve the dependencies in the functions and procedures,
> so that the exported file, when re-played could lead to invalid objects.
> 
> So, I decided this would not make Postgres better and decide to walk off
> this patch.
> 
> Anyhow, when sharing my thoughts, several people told me to ask the
> community about adding this feature because this could be useful in some
> use cases. Another argument is that should you have all your functions in
> one schema and your tables in another, exporting only the function schema
> will lead to the same kind of file that could lead to invalid objects
> created when the file is re-played against a database that does not have
> the tables.
> 
> Of course, the documentation would add a warning against object invalidity
> should only the stored functions/procedures be exported.
> 
> So, my question is: what do you think about such a feature? is it worth it?

Yes, it is absolutely worth it to be able to extract just the functions
from a database.  I have wanted this several times.
-- 
Vik Fearing

Vik Fearing <vik@postgresfriends.org> writes:
> On 3/17/21 6:00 PM, Lætitia Avrot wrote:
>> However, when I finally got the time to look at it in detail, I found out
>> there was no way to solve the dependencies in the functions and procedures,
>> so that the exported file, when re-played could lead to invalid objects.
>>  ...
>> So, my question is: what do you think about such a feature? is it worth it?

> Yes, it is absolutely worth it to be able to extract just the functions
> from a database.  I have wanted this several times.

Selective dumps always have a risk of not being restorable on their
own; I don't see that "functions only" is noticeably less safe than
"just these tables", or other cases that we support already.

What I'm wondering about is how this might interact with the
discussion at [1].

            regards, tom lane

[1] https://www.postgresql.org/message-id/flat/CAFj8pRB10wvW0CC9Xq=1XDs=zCQxer3cbLcNZa+qiX4cUH-G_A@mail.gmail.com

On 3/27/21 8:22 AM, Lætitia Avrot wrote:
> Hello,
>
> You'll find enclosed the first version of my patch. I did not include
> the possibility of using a file to list tables to be exported as Tom
> suggested because I genuinely think it is a totally different matter.
> It does not mean I'm not open to the possibility, it just felt weird.
>
> The patch allows using a `--functions-only` flag in `pg_dump` to
> export only functions and stored procedures. My code was build and
> passed tests on the last master branch of the PostgreSQL project. I
> added regression tests. Documentation has been updated too and
> generation of the documentation (HTML, man page, pdf in A4 and letter
> US format) has been tested successfully.


We can bikeshed the name of the flag at some stage. --procedures-only
might also make sense


>
> I did not add a warning in the documentation that the file provided
> might end up in a not restorable file or in a file restoring broken
> functions or procedures. Do you think I should?


No, I don't think it's any different from any of the other similar switches.


>
> I don't know if this patch has any impact on performance. I guess that
> adding 4 if statements will slow down `pg_dump` a little bit.
>
>

Not likely to be noticeable.


Please add this to the next commitfest.


cheers


andrew

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

The following review has been posted through the commitfest application:
make installcheck-world:  tested, failed
Implements feature:       tested, passed
Spec compliant:           tested, passed
Documentation:            not tested

Few minor comments :

- The latest patch has some hunk failures
- Regression with master has many failures with/without the patch, it is difficult to tell if the patch is causing any
failures.
- This is probably intended behaviour that --functions-only switch is also dumping stored procedures? 
- If i create a procedure with 
LANGUAGE plpgsql
SECURITY INVOKER
It is not including "SECURITY INVOKER" in the dump. That's probably because INVOKER is default security rights.

Hi,

I took a quick look at the patch today. There was some minor bitrot
requiring a rebase, so I attach the rebased patch as v3.

The separate 0002 part contains some minor fixes - a couple
typos/rewording in the docs (would be good if a native speaker looked at
it, thought), and a slightly reworked chunk of code from pg_dump.c. The
change is more a matter of personal preference than correctness - it
just seems simpler this way, but ymmv. And I disliked that the comment
said "If we have to export only the functions .." but the if condition
was actually the exact opposite of that.

The main question I have is whether this should include procedures. I'd
probably argue procedures should be considered different from functions
(i.e. requiring a separate --procedures-only option), because it pretty
much is meant to be a separate object type. We don't allow calling DROP
FUNCTION on a procedure, etc. It'd be silly to introduce an unnecessary
ambiguity in pg_dump and have to deal with it sometime later.

I wonder if we should allow naming a function to dump, similarly to how
--table works for tables, for example.


regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Tomas Vondra <tomas.vondra@enterprisedb.com> writes:
> The main question I have is whether this should include procedures.

I feel a bit uncomfortable about sticking this sort of limited-purpose
selectivity mechanism into pg_dump.  I'd rather see a general filter
method that can select object(s) of any type.  Pavel was doing some
work towards that awhile ago, though I think he got frustrated about
the lack of consensus on details.  Which is a problem, but I don't
think the solution is to accrue more and more independently-designed-
and-implemented features that each solve some subset of the big problem.

            regards, tom lane

On 7/10/21 1:43 AM, Tom Lane wrote:
> Tomas Vondra <tomas.vondra@enterprisedb.com> writes:
>> The main question I have is whether this should include procedures.
> 
> I feel a bit uncomfortable about sticking this sort of limited-purpose
> selectivity mechanism into pg_dump.  I'd rather see a general filter
> method that can select object(s) of any type.  Pavel was doing some
> work towards that awhile ago, though I think he got frustrated about
> the lack of consensus on details.  Which is a problem, but I don't
> think the solution is to accrue more and more independently-designed-
> and-implemented features that each solve some subset of the big problem.
> 

I'm not against introducing such general filter mechanism, but why 
should it block this patch? I'd understand it the patch was adding a lot 
of code, but that's not the case - it's tiny. And we already have 
multiple filter options (to pick tables, schemas, extensions, ...).

And if there's no consensus on details of Pavel's patch after multiple 
commitfests, how likely is it it'll start moving forward?


regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

> On 30 Jul 2021, at 12:55, Lætitia Avrot <laetitia.avrot@gmail.com> wrote:
>
> > On Fri, Jul 9, 2021 at 4:43 PM Tomas Vondra <tomas.vondra@enterprisedb.com> wrote:
> >
> > The main question I have is whether this should include procedures. I'd
> > probably argue procedures should be considered different from functions
> > (i.e. requiring a separate --procedures-only option), because it pretty
> > much is meant to be a separate object type. We don't allow calling DROP
> > FUNCTION on a procedure, etc. It'd be silly to introduce an unnecessary
> > ambiguity in pg_dump and have to deal with it sometime later.
>
> I respectfully disagree. In psql, the `\ef` and `\df` metacommands will also list procedures, not just functions.

I tend to agree that we should include both, while they are clearly different I
don't think it would be helpful in this case to distinguish.

Looking at this thread I think it makes sense to go ahead with this patch.  The
filter functionality worked on in another thread is dealing with cherry-picking
certain objects where this is an all-or-nothing switch, so I don't think they
are at odds with each other.

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

On Tue, Nov 09, 2021 at 03:23:07PM +0100, Daniel Gustafsson wrote:
> Looking at this thread I think it makes sense to go ahead with this patch.  The
> filter functionality worked on in another thread is dealing with cherry-picking
> certain objects where this is an all-or-nothing switch, so I don't think they
> are at odds with each other.

Including both procedures and functions sounds natural from here.  Now
I have a different question, something that has not been discussed in
this thread at all.  What about patterns?  Switches like --table or
--extension are able to digest a psql-like pattern to decide which
objects to dump.  Is there a reason not to have this capability for
this new switch with procedure names?  I mean to handle the case
without the function arguments, even if the same name is used by
multiple functions with different arguments.
--
Michael

It looks like this discussion has reached a bit of an impasse with Tom
being against this approach and Michael and Daniel being for it. It
doesn't look like it's going to get committed this commitfest, shall
we move it forward or mark it returned with feedback?

> On 24 Mar 2022, at 23:38, Greg Stark <stark@mit.edu> wrote:

> It looks like this discussion has reached a bit of an impasse with Tom
> being against this approach and Michael and Daniel being for it. It
> doesn't look like it's going to get committed this commitfest, shall
> we move it forward or mark it returned with feedback?

Lætitia mentioned the other day off-list that she was going to try and update
this patch with the pattern support proposed, so hopefully we will hear from
her shortly on that.

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

Daniel Gustafsson <daniel@yesql.se> writes:
>> On 24 Mar 2022, at 23:38, Greg Stark <stark@mit.edu> wrote:
>> It looks like this discussion has reached a bit of an impasse with Tom
>> being against this approach and Michael and Daniel being for it. It
>> doesn't look like it's going to get committed this commitfest, shall
>> we move it forward or mark it returned with feedback?

> Lætitia mentioned the other day off-list that she was going to try and update
> this patch with the pattern support proposed, so hopefully we will hear from
> her shortly on that.

To clarify: I'm not against having an easy way to dump all-and-only
functions.  What concerns me is having such a feature that's designed
in isolation, without a plan for anything else.  I'd like to create
some sort of road map for future selective-dumping options, and then
we can make sure that this feature fits into the bigger picture.
Otherwise we're going to end up with an accumulation of warts, with
inconsistent naming and syntax, and who knows what other sources of
confusion.

            regards, tom lane

On 03/27/21 08:57, Andrew Dunstan wrote:
> We can bikeshed the name of the flag at some stage. --procedures-only
> might also make sense

Any takers for --routines-only ?

"Routine" is the genuine, ISO SQL umbrella term for a function or
procedure, and we have used it that way in our docs and glossary.

Regards,
-Chap

"David G. Johnston" <david.g.johnston@gmail.com> writes:
> The extension object type does not seem to have gotten the
> --exclude-extension capability that it would need to conform to the general
> design exemplified by --table and hopefully extended out to the routine
> object types.

We're not going to instantly build out every feature that would be
suggested by a roadmap.  However, I see in what you just wrote
a plausible roadmap: eventually, all or most object types should
have pg_dump switches comparable to, and syntactically aligned
with, the --table and --exclude-table switches.  The expectation
would be that if any of these selective-dump switches appear,
then only objects matching at least one of them (and not matching
any --exclude switch) will be dumped.  So for example

    pg_dump --table=foo* --function=bar*

dumps tables whose names start with foo, and functions whose
names start with bar, and nothing else.  (We'd need to spell out
how these things interact with --schema, too.)

In this scheme, Lætitia's desired functionality should be spelled
"--function=*", or possibly "--routine=*", depending on what she
wanted to happen with procedures.

Thoughts?

            regards, tom lane

Laetitia Avrot <laetitia.avrot@gmail.com> writes:
> Thank you so much for your suggestion. I was really excited to find a
> generic term for Functions and Procedures, but "routine" also includes
> aggregation functions which I had excluded from my feature (see Postgres
> Glossary here:
> https://www.postgresql.org/docs/14/glossary.html#GLOSSARY-ROUTINE).

> I had decided not to include aggregate functions when I designed my patch
> because I thought most users wouldn't expect them in the result file. Was I
> wrong?

I'd vote for treating them as functions for this purpose.  I'd put
them in the same category as window functions: we use a separate
name for them for historical reasons, but they still walk and quack
pretty much like functions.

            regards, tom lane

"David G. Johnston" <david.g.johnston@gmail.com> writes:
> I don't find the --objectype-only option to be desirable.  psql
> --tables-only --functions-only just seems odd, no longer are they "only".
> I would go with --function-all (and maybe --function-system and
> --function-user) if going down this path but the wildcard feature can
> handle this just fine and we want that feature anyway.

Agreed.  "--function=*" is more general than "--function-only",
and shorter too, so what's not to like?

> Except succinctly
> omitting system objects which should get its own general option.

pg_dump never dumps system objects, so I don't see a need for
a switch to tell it not to.

            regards, tom lane

"David G. Johnston" <david.g.johnston@gmail.com> writes:
> On Fri, Mar 25, 2022 at 10:57 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> pg_dump never dumps system objects, so I don't see a need for
>> a switch to tell it not to.

> I considered pg_class to be a system object, which was dumped under -t '*'

Oh!  You're right, the --table switches will include system objects.
That seems like a bug TBH.  Even if it's intentional, it's surely
not behavior we want for functions.  You can somewhat easily
exclude system catalogs from matching --table since they all have
names starting with "pg_", but it'd be way more painful for functions
because (a) there are thousands and (b) they're not very predictably
named.

I'd vote for changing the behavior of --table rather than trying to
be bug-compatible with this decision.

            regards, tom lane

"David G. Johnston" <david.g.johnston@gmail.com> writes:
> On Fri, Mar 25, 2022 at 10:57 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> pg_dump never dumps system objects, so I don't see a need for
>> a switch to tell it not to.

> I considered pg_class to be a system object, which was dumped under -t '*'

I'd vote for changing the behavior of --table rather than trying to
be bug-compatible with this decision.

> On 25 Mar 2022, at 01:40, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> "David G. Johnston" <david.g.johnston@gmail.com> writes:
>> The extension object type does not seem to have gotten the
>> --exclude-extension capability that it would need to conform to the general
>> design exemplified by --table and hopefully extended out to the routine
>> object types.
>
> We're not going to instantly build out every feature that would be
> suggested by a roadmap.

Agreed.  In this case it seems that adding --exclude-extension would make sense
to keep conistency.  I took a quick stab at doing so with the attached while
we're here.

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

> On 25 Mar 2022, at 19:37, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> I'd vote for changing the behavior of --table rather than trying to
> be bug-compatible with this decision.

Agreed.  Question is what to do for "-t pg_class", should we still forbid
dumping system catalogs when they are pattern matched without wildcard or is
should that be ok? And should this depend on if "-n pg_catalog" is used?

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

Daniel Gustafsson <daniel@yesql.se> writes:
>> On 25 Mar 2022, at 19:37, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I'd vote for changing the behavior of --table rather than trying to
>> be bug-compatible with this decision.

> Agreed.  Question is what to do for "-t pg_class", should we still forbid
> dumping system catalogs when they are pattern matched without wildcard or is
> should that be ok? And should this depend on if "-n pg_catalog" is used?

I don't think there's anything really wrong with just "we won't dump
system objects, full stop"; I don't see much use-case for doing that
except maybe debugging, and even that is a pretty thin argument.

However, a possible compromise is to say that we act as though
--exclude-schema=pg_catalog is specified unless you explicitly
override that with "--schema=pg_catalog".  (And the same for
information_schema, I suppose.)  This might be a bit hacky to
implement :-(

            regards, tom lane

"David G. Johnston" <david.g.johnston@gmail.com> writes:
> If we want to choose the other position I would just go with
> "--[no]-system-objects" options to toggle whether pattern matching grabs
> them by default (defaulting to no) and if someone wants to enable them for
> only specific object types they can --system-objects and then
> --exclude-type='pg_catalog' any that shouldn't be enabled.

Yeah, I could live with that.  Per-object-type control doesn't
seem necessary.

            regards, tom lane

On Fri, Mar 25, 2022 at 10:09:33PM +0100, Daniel Gustafsson wrote:
> Agreed.  In this case it seems that adding --exclude-extension would make sense
> to keep conistency.  I took a quick stab at doing so with the attached while
> we're here.

src/test/modules/test_pg_dump would be the best place for the addition
of a couple of tests with this new switch.  Better to check as well
what happens when a command collides with --extension and
--exclude-extension.

    printf(_("  -e, --extension=PATTERN      dump the specified extension(s) only\n"));
+   printf(_("  --exclude-extension=PATTERN  do NOT dump the specified extension(s)\n"));
Shouldn't this be listed closer to --exclude-table-data in the --help
output?
--
Michael

On Sat, Mar 26, 2022 at 09:53:19AM +0100, Laetitia Avrot wrote:
> I think it's time to sum up what we want to do:
>
> - We'd like to use switches to export objects according to a pattern.
> - For each object type we will have an --object=PATTERN flag and a
> --exclude-object=PATTERN
> - Having a short flag for each of the long flags is not mandatory
> - The object types that pg_dump can select so far are:
>     - table (already written)
>     - schema (already written)

>     - extension (half-written, --exclude-extension not written)

I would be to blame on this item.

>     - routine (TBD ASAP). Routine flag operates on stored functions, stored
> procedures, aggregate functions, and window functions.
> - By default, pg_dump does not export system objects but we found out that
> we could use --table='pg_catalog.*' to export them. This is a bug and will
> be fixed. pg_dump won't have the ability to export any system object
> anymore. Should the fix belong to that patch or do I need to create a
> separate patch? (Seems to me it should be separated)
>
> If everyone is ok with the points above, I'll write both patches.

Looks clear to me that a different design is wanted here, and that
this won't make it for v15, so I have marked the patch as returned
with feedback in the CF app.
--
Michael