Re: [HACKERS] Patch: Add --no-comments to skip COMMENTs with pg_dump

2017-09-12 Thread Robert Haas
On Mon, Sep 11, 2017 at 6:41 PM, Michael Paquier
 wrote:
> On Mon, Sep 11, 2017 at 11:43 PM, Robert Haas  wrote:
>> So I think this is just an excuse for turning --no-security-labels
>> into --no-object-property=security-label.  To me, that's just plain
>> worse.
>
> It does not seem that my thoughts here have been correctly transmitted
> to your brain.

Dang, we really should have put more work into that inter-brain link.
PostgreSQL group mind FTW!

> I do not mean to change the user-facing options, just
> to refactor the code internally so as --no-foo switches can be more
> easily generated, added and handled as they are associated with an
> object type. A portion of the complains is caused by the fact that a
> lot of similar code is duplicated.

Ah, well.  No objection to refactoring away duplicate code, of course.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Patch: Add --no-comments to skip COMMENTs with pg_dump

2017-09-11 Thread Michael Paquier
On Mon, Sep 11, 2017 at 11:43 PM, Robert Haas  wrote:
> So I think this is just an excuse for turning --no-security-labels
> into --no-object-property=security-label.  To me, that's just plain
> worse.

It does not seem that my thoughts here have been correctly transmitted
to your brain. I do not mean to change the user-facing options, just
to refactor the code internally so as --no-foo switches can be more
easily generated, added and handled as they are associated with an
object type. A portion of the complains is caused by the fact that a
lot of similar code is duplicated.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Patch: Add --no-comments to skip COMMENTs with pg_dump

2017-09-11 Thread Robert Haas
On Sun, Sep 10, 2017 at 6:25 PM, Stephen Frost  wrote:
> * Michael Paquier (michael.paqu...@gmail.com) wrote:
>> As there begins to be many switches of this kind and much code
>> duplication, I think that some refactoring into a more generic switch
>> infrastructure would be nicer.
>
> I have been thinking about this also and agree that it would be nicer,
> but then these options would just become "shorthand" for the generic
> switch.

I don't really like the "generic switch infrastructure" concept.  I
think it will make specifying behaviors harder without any
corresponding benefit.  There are quite a few --no-xxx switches now
but the total number of them that we could conceivably end up with
doesn't seem to be a lot bigger than what we have already.

Now, if we want switches to exclude a certain kind of object (e.g.
table, function, text search configuration) from the backup
altogether, that should be done in some generic way, like
--exclude-object-type=table.  But that's not what this is about.  This
is about excluding a certain kind of property (comments, in this case)
from being backed up.  And an individual kind of object doesn't have
many more properties than what we already handle.

So I think this is just an excuse for turning --no-security-labels
into --no-object-property=security-label.  To me, that's just plain
worse.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Patch: Add --no-comments to skip COMMENTs with pg_dump

2017-09-10 Thread Stephen Frost
Michael,

* Michael Paquier (michael.paqu...@gmail.com) wrote:
> As there begins to be many switches of this kind and much code
> duplication, I think that some refactoring into a more generic switch
> infrastructure would be nicer.

I have been thinking about this also and agree that it would be nicer,
but then these options would just become "shorthand" for the generic
switch.

Thanks!

Stephen


signature.asc
Description: Digital signature


Re: [HACKERS] Patch: Add --no-comments to skip COMMENTs with pg_dump

2017-09-06 Thread Michael Paquier
On Thu, Sep 7, 2017 at 1:43 AM, Robert Haas  wrote:
> On Wed, Sep 6, 2017 at 12:26 PM, Simon Riggs  wrote:
 I'd personally be fine with --no-whatever for any whatever that might
 be a subsidiary property of database objects.  We've got
 --no-security-labels, --no-tablespaces, --no-owner, and
 --no-privileges already, so what's wrong with --no-comments?

 (We've also got --no-publications; I think it's arguable whether that
 is the same kind of thing.)
>>>
>>> And --no-subscriptions in the same bucket.
>>
>> Yes, it is. I was suggesting that we remove those as well.

FWIW, I do too. They are useful for given application code paths.

> That seems like a non-starter to me.  I have used those options many
> times to solve real problems, and I'm sure other people have as well.
> We wouldn't have ended up with all of these options if users didn't
> want to control such things.

As there begins to be many switches of this kind and much code
duplication, I think that some refactoring into a more generic switch
infrastructure would be nicer.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Patch: Add --no-comments to skip COMMENTs with pg_dump

2017-09-06 Thread Robert Haas
On Wed, Sep 6, 2017 at 12:26 PM, Simon Riggs  wrote:
>>> I'd personally be fine with --no-whatever for any whatever that might
>>> be a subsidiary property of database objects.  We've got
>>> --no-security-labels, --no-tablespaces, --no-owner, and
>>> --no-privileges already, so what's wrong with --no-comments?
>>>
>>> (We've also got --no-publications; I think it's arguable whether that
>>> is the same kind of thing.)
>>
>> And --no-subscriptions in the same bucket.
>
> Yes, it is. I was suggesting that we remove those as well.

That seems like a non-starter to me.  I have used those options many
times to solve real problems, and I'm sure other people have as well.
We wouldn't have ended up with all of these options if users didn't
want to control such things.

> But back to the main point which is that --no-comments discards ALL
> comments simply to exclude one pointless and annoying comment. That
> runs counter to our stance that we do not allow silent data loss. I
> want to solve the problem too. I accept that not everyone uses
> comments, but if they do, spilling them all on the floor is a user
> visible slip up that we should not be encouraging. Sorry y'all.

/me shrugs.

I agree that there could be better solutions to the original problem,
but I think there's no real argument that a user can't exclude
comments from a backup if they wish to do so.  As the OP already
pointed out, it can still be done without the switch; it's just more
annoying.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Patch: Add --no-comments to skip COMMENTs with pg_dump

2017-09-06 Thread Simon Riggs
On 1 September 2017 at 22:08, Michael Paquier  wrote:
> On Sat, Sep 2, 2017 at 1:53 AM, Robert Haas  wrote:
>> On Mon, Aug 21, 2017 at 5:30 PM, Simon Riggs  wrote:
>>> Thinking ahead, are we going to add a new --no-objecttype switch every
>>> time someone wants it?
>>
>> I'd personally be fine with --no-whatever for any whatever that might
>> be a subsidiary property of database objects.  We've got
>> --no-security-labels, --no-tablespaces, --no-owner, and
>> --no-privileges already, so what's wrong with --no-comments?
>>
>> (We've also got --no-publications; I think it's arguable whether that
>> is the same kind of thing.)
>
> And --no-subscriptions in the same bucket.

Yes, it is. I was suggesting that we remove those as well.

But back to the main point which is that --no-comments discards ALL
comments simply to exclude one pointless and annoying comment. That
runs counter to our stance that we do not allow silent data loss. I
want to solve the problem too. I accept that not everyone uses
comments, but if they do, spilling them all on the floor is a user
visible slip up that we should not be encouraging. Sorry y'all.

-- 
Simon Riggshttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Patch: Add --no-comments to skip COMMENTs with pg_dump

2017-09-01 Thread Michael Paquier
On Sat, Sep 2, 2017 at 1:53 AM, Robert Haas  wrote:
> On Mon, Aug 21, 2017 at 5:30 PM, Simon Riggs  wrote:
>> Thinking ahead, are we going to add a new --no-objecttype switch every
>> time someone wants it?
>
> I'd personally be fine with --no-whatever for any whatever that might
> be a subsidiary property of database objects.  We've got
> --no-security-labels, --no-tablespaces, --no-owner, and
> --no-privileges already, so what's wrong with --no-comments?
>
> (We've also got --no-publications; I think it's arguable whether that
> is the same kind of thing.)

And --no-subscriptions in the same bucket.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Patch: Add --no-comments to skip COMMENTs with pg_dump

2017-09-01 Thread Robert Haas
On Mon, Aug 21, 2017 at 5:30 PM, Simon Riggs  wrote:
> Thinking ahead, are we going to add a new --no-objecttype switch every
> time someone wants it?

I'd personally be fine with --no-whatever for any whatever that might
be a subsidiary property of database objects.  We've got
--no-security-labels, --no-tablespaces, --no-owner, and
--no-privileges already, so what's wrong with --no-comments?

(We've also got --no-publications; I think it's arguable whether that
is the same kind of thing.)

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Patch: Add --no-comments to skip COMMENTs with pg_dump

2017-08-21 Thread David G. Johnston
On Mon, Aug 21, 2017 at 2:30 PM, Simon Riggs  wrote:

>
> > The patch applies cleanly to current master and all tests run without
> > failures.
> >
> > I also test against all current supported versions (9.2 ... 9.6) and
> didn't
> > find any issue.
> >
> > Changed status to "ready for commiter".
>
> I get the problem, but not this solution. It's too specific and of
> zero other value, yet not even exactly specific to the issue. We
> definitely don't want --no-extension-comments, but --no-comments
> removes ALL comments just to solve a weird problem. (Meta)Data loss,
> surely?
>

​It was argued, successfully IMO, to have this capability independent of
any particular problem to be solved.  In that context I haven't given the
proposed implementation much thought.

I do agree that this is a very unappealing solution for the stated problem
and am hoping someone will either have an ingenious idea to solve it the
right way or is willing to hold their nose and put in a hack.

David J.


Re: [HACKERS] Patch: Add --no-comments to skip COMMENTs with pg_dump

2017-08-21 Thread Simon Riggs
On 7 August 2017 at 16:14, Fabrízio de Royes Mello
 wrote:
>
> On Mon, Aug 7, 2017 at 10:43 AM, Robins Tharakan  wrote:
>>
>> On 20 July 2017 at 05:14, Robins Tharakan  wrote:
>>>
>>> On 20 July 2017 at 05:08, Michael Paquier 
>>> wrote:

 On Wed, Jul 19, 2017 at 8:59 PM,
 Fabrízio de Royes Mello
 > You should add the properly sgml docs for this pg_dumpall change also.

 Tests of pg_dump go to src/bin/pg_dump/t/ and tests for objects in
 extensions are in src/test/modules/test_pg_dump, but you just care
 about the former with this patch. And if you implement some new tests,
 look at the other tests and base your work on that.
>>>
>>>
>>> Thanks Michael /
>>> Fabrízio.
>>>
>>> Updated patch (attached) additionally adds SGML changes for pg_dumpall.
>>> (I'll try to work on the tests, but sending this
>>> nonetheless
>>> ).
>>>
>>
>> Attached is an updated patch (v4) with basic tests for pg_dump /
>> pg_dumpall.
>> (Have zipped it since patch size jumped to ~40kb).
>>
>
> The patch applies cleanly to current master and all tests run without
> failures.
>
> I also test against all current supported versions (9.2 ... 9.6) and didn't
> find any issue.
>
> Changed status to "ready for commiter".

I get the problem, but not this solution. It's too specific and of
zero other value, yet not even exactly specific to the issue. We
definitely don't want --no-extension-comments, but --no-comments
removes ALL comments just to solve a weird problem. (Meta)Data loss,
surely?

Thinking ahead, are we going to add a new --no-objecttype switch every
time someone wants it?

It would make more sense to add something more general and extensible
such as --exclude-objects=comment
or similar name

-- 
Simon Riggshttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Patch: Add --no-comments to skip COMMENTs with pg_dump

2017-08-07 Thread Fabrízio de Royes Mello
On Mon, Aug 7, 2017 at 10:43 AM, Robins Tharakan  wrote:
>
> On 20 July 2017 at 05:14, Robins Tharakan  wrote:
>>
>> On 20 July 2017 at 05:08, Michael Paquier 
wrote:
>>>
>>> On Wed, Jul 19, 2017 at 8:59 PM,
>>> Fabrízio de Royes Mello
>>> > You should add the properly sgml docs for this pg_dumpall change also.
>>>
>>> Tests of pg_dump go to src/bin/pg_dump/t/ and tests for objects in
>>> extensions are in src/test/modules/test_pg_dump, but you just care
>>> about the former with this patch. And if you implement some new tests,
>>> look at the other tests and base your work on that.
>>
>>
>> Thanks Michael /
>> Fabrízio.
>>
>> Updated patch (attached) additionally adds SGML changes for pg_dumpall.
>> (I'll try to work on the tests, but sending this
>> nonetheless
>> ).
>>
>
> Attached is an updated patch (v4) with basic tests for pg_dump /
pg_dumpall.
> (Have zipped it since patch size jumped to ~40kb).
>

The patch applies cleanly to current master and all tests run without
failures.

I also test against all current supported versions (9.2 ... 9.6) and didn't
find any issue.

Changed status to "ready for commiter".

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello


Re: [HACKERS] Patch: Add --no-comments to skip COMMENTs with pg_dump

2017-07-19 Thread Robins Tharakan
On 20 July 2017 at 05:08, Michael Paquier  wrote:

> On Wed, Jul 19, 2017 at 8:59 PM,
> ​​
> Fabrízio de Royes Mello
> > You should add the properly sgml docs for this pg_dumpall change also.
>
> Tests of pg_dump go to src/bin/pg_dump/t/ and tests for objects in
> extensions are in src/test/modules/test_pg_dump, but you just care
> about the former with this patch. And if you implement some new tests,
> look at the other tests and base your work on that.
>

​Thanks Michael /
​
Fabrízio.

Updated patch (attached) additionally adds SGML changes for pg_dumpall.
(I'll try to work on the tests, but sending this
​​
nonetheless
​).​


-
robins


pgdump_nocomments_v3.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Patch: Add --no-comments to skip COMMENTs with pg_dump

2017-07-19 Thread Michael Paquier
On Wed, Jul 19, 2017 at 8:59 PM, Fabrízio de Royes Mello
 wrote:
> On Wed, Jul 19, 2017 at 3:54 PM, Robins Tharakan  wrote:
>> You may want to consider this patch (attached) which additionally has the
>> pg_dumpall changes.
>> It would be great if you could help with the tests though, am unsure how
>> to go about them.
>
> You should add the properly sgml docs for this pg_dumpall change also.

Tests of pg_dump go to src/bin/pg_dump/t/ and tests for objects in
extensions are in src/test/modules/test_pg_dump, but you just care
about the former with this patch. And if you implement some new tests,
look at the other tests and base your work on that.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Patch: Add --no-comments to skip COMMENTs with pg_dump

2017-07-19 Thread Fabrízio de Royes Mello
On Wed, Jul 19, 2017 at 3:54 PM, Robins Tharakan  wrote:
>
>
> On 18 July 2017 at 23:55, David Fetter  wrote:
>>
>> Excellent point about pg_dumpall.  I'll see what I can draft up in the
>> next day or two and report back.
>
>
>
> Hi David,
>
> You may want to consider this patch (attached) which additionally has the
pg_dumpall changes.
> It would be great if you could help with the tests though, am unsure how
to go about them.
>

You should add the properly sgml docs for this pg_dumpall change also.

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello


Re: [HACKERS] Patch: Add --no-comments to skip COMMENTs with pg_dump

2017-07-19 Thread Robins Tharakan
On 18 July 2017 at 23:55, David Fetter  wrote:
>
> Excellent point about pg_dumpall.  I'll see what I can draft up in the
> next day or two and report back.



​Hi David,

You may want to consider this patch (attached) which additionally has the
pg_dumpall changes.
It would be great if you could help with the tests though, am unsure how to
go about them.

-
robins​


pgdump_nocomments_v2.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Patch: Add --no-comments to skip COMMENTs with pg_dump

2017-07-18 Thread David Fetter
On Tue, Jul 18, 2017 at 08:38:25AM +0200, Michael Paquier wrote:
> On Tue, Jul 18, 2017 at 3:45 AM, David Fetter  wrote:
> > The one I run into frequently is in a proprietary fork, RDS Postgres.
> > It'll happily dump out COMMENT ON EXTENSION plpgsq IS ...
> > which is great as far as it goes, but errors out when you try to
> > reload it.
> >
> > While bending over backwards to support proprietary forks strikes me
> > as a terrible idea, I'd like to enable pg_dump to produce and consume
> > ToCs just as pg_restore does with its -l/-L options.  This would
> > provide the finest possible grain.
> 
> Let's add as well a similar switch to pg_dumpall that gets pushed down
> to all the created pg_dump commands. If this patch gets integrated
> as-is this is going to be asked. And tests would be welcome.

Excellent point about pg_dumpall.  I'll see what I can draft up in the
next day or two and report back.

Best,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter  XMPP: david(dot)fetter(at)gmail(dot)com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Patch: Add --no-comments to skip COMMENTs with pg_dump

2017-07-18 Thread Michael Paquier
On Tue, Jul 18, 2017 at 3:45 AM, David Fetter  wrote:
> The one I run into frequently is in a proprietary fork, RDS Postgres.
> It'll happily dump out COMMENT ON EXTENSION plpgsq IS ...
> which is great as far as it goes, but errors out when you try to
> reload it.
>
> While bending over backwards to support proprietary forks strikes me
> as a terrible idea, I'd like to enable pg_dump to produce and consume
> ToCs just as pg_restore does with its -l/-L options.  This would
> provide the finest possible grain.

Let's add as well a similar switch to pg_dumpall that gets pushed down
to all the created pg_dump commands. If this patch gets integrated
as-is this is going to be asked. And tests would be welcome.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Patch: Add --no-comments to skip COMMENTs with pg_dump

2017-07-17 Thread David Fetter
On Thu, Jun 01, 2017 at 10:05:09PM -0400, Tom Lane wrote:
> Robert Haas  writes:
> > On Tue, May 30, 2017 at 8:55 PM, David G. Johnston
> >  wrote:
> >>> Having --no-comments seems generally useful to me, in any case.
> 
> >> It smacks of being excessive to me.
> 
> > It sounds perfectly sensible to me.  It's not exactly an elegant
> > solution to the original problem, but it's a reasonable switch on
> > its own merits.
> 
> I dunno.  What's the actual use-case, other than as a bad workaround
> to a problem we should fix a different way?

The one I run into frequently is in a proprietary fork, RDS Postgres.
It'll happily dump out COMMENT ON EXTENSION plpgsq IS ...
which is great as far as it goes, but errors out when you try to
reload it.

While bending over backwards to support proprietary forks strikes me
as a terrible idea, I'd like to enable pg_dump to produce and consume
ToCs just as pg_restore does with its -l/-L options.  This would
provide the finest possible grain.

Best,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter  XMPP: david(dot)fetter(at)gmail(dot)com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Patch: Add --no-comments to skip COMMENTs with pg_dump

2017-07-17 Thread Fabrízio Mello
The following review has been posted through the commitfest application:
make installcheck-world:  not tested
Implements feature:   tested, passed
Spec compliant:   not tested
Documentation:tested, passed

It's a very simple change and I have not to complain about source and 
documentation changes.

But I wonder the lack of tap tests of this new pg_dump behavior. Shouldn't we 
add tap tests?

The new status of this patch is: Waiting on Author

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Patch: Add --no-comments to skip COMMENTs with pg_dump

2017-06-02 Thread Robert Haas
On Thu, Jun 1, 2017 at 10:05 PM, Tom Lane  wrote:
> I dunno.  What's the actual use-case, other than as a bad workaround
> to a problem we should fix a different way?

Well, that's a fair point.  I don't have a specific use case in mind.
However, I also don't think that options for controlling what gets
dumped should be subjected to extreme vetting.  On the strength mostly
of my own experiences trying to solve database problems in the real
world, I generally think that pg_dump could benefit from significantly
more options to control what gets dumped.  The existing options are
pretty good, but it's not that hard to run into a situation that they
don't cover.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Patch: Add --no-comments to skip COMMENTs with pg_dump

2017-06-01 Thread Stephen Frost
Tom,

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Robert Haas  writes:
> > On Tue, May 30, 2017 at 8:55 PM, David G. Johnston
> >  wrote:
> >>> Having --no-comments seems generally useful to me, in any case.
> 
> >> It smacks of being excessive to me.
> 
> > It sounds perfectly sensible to me.  It's not exactly an elegant
> > solution to the original problem, but it's a reasonable switch on its
> > own merits.
> 
> I dunno.  What's the actual use-case, other than as a bad workaround
> to a problem we should fix a different way?

Perhaps it's a bit of a stretch, I'll admit, but certainly "minmization"
and "obfuscation" come to mind, which are often done in other fields and
might well apply in very specific cases to PG schemas.  I can certainly
also see a case being made that you'd like to extract a schema-only dump
which doesn't include any comments because the comments have information
that you'd rather not share publicly, while the schema itself is fine to
share.  Again, a bit of a stretch, but not unreasonable.

Otherwise, well, for my 2c anyway, feels like it's simply fleshing out
the options which correspond to the different components of an object.
We provide similar for ACLs, security labels, and tablespace
association.  If there are other components of an object which we should
consider adding an option to exclude, I'm all ears, personally
(indexes?).  Also, with the changes that I've made to pg_dump, I'm
hopeful that such options will end up requiring a very minor amount of
code to implement.  There's more work to be done in that area too,
certainly, but I do feel like it's better than it was.

I definitely would like to see more flexibility in this area in general.

Thanks!

Stephen


signature.asc
Description: Digital signature


Re: [HACKERS] Patch: Add --no-comments to skip COMMENTs with pg_dump

2017-06-01 Thread Tom Lane
Robert Haas  writes:
> On Tue, May 30, 2017 at 8:55 PM, David G. Johnston
>  wrote:
>>> Having --no-comments seems generally useful to me, in any case.

>> It smacks of being excessive to me.

> It sounds perfectly sensible to me.  It's not exactly an elegant
> solution to the original problem, but it's a reasonable switch on its
> own merits.

I dunno.  What's the actual use-case, other than as a bad workaround
to a problem we should fix a different way?

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Patch: Add --no-comments to skip COMMENTs with pg_dump

2017-06-01 Thread Robert Haas
On Tue, May 30, 2017 at 8:55 PM, David G. Johnston
 wrote:
>> Having --no-comments seems generally useful to me, in any case.
>
> It smacks of being excessive to me.

It sounds perfectly sensible to me.  It's not exactly an elegant
solution to the original problem, but it's a reasonable switch on its
own merits.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Patch: Add --no-comments to skip COMMENTs with pg_dump

2017-05-31 Thread Stephen Frost
David,

* David G. Johnston (david.g.johns...@gmail.com) wrote:
> On Tue, May 30, 2017 at 8:41 PM, Stephen Frost  wrote:
> > * David G. Johnston (david.g.johns...@gmail.com) wrote:
> > > On Fri, May 26, 2017 at 7:47 AM, Stephen Frost 
> > wrote:
> > > > * Robins Tharakan (thara...@gmail.com) wrote:
> > > > > Attached is a patch adds a --no-comments argument to pg_dump to skip
> > > > > generation of COMMENT statements when generating a backup. This is
> > > > crucial
> > > > > for non-superusers to restore a database backup in a Single
> > Transaction.
> > > > > Currently, this requires one to remove COMMENTs via scripts, which is
> > > > > inelegant at best.
> > > >
> > > > Having --no-comments seems generally useful to me, in any case.
> > >
> > > I​t smacks of being excessive to me.
> > > ​
> >
> > I have a hard time having an issue with an option that exists to exclude
> > a particular type of object from being in the dump.
> 
> ​Excessive with respect to the problem at hand.

Fair enough.  I tend to agree with that sentiment.

> A single comment in the
> dump is unable to be restored.  Because of that we are going to require
> people to omit every comment in their database.  The loss of all that
> information is in excess of what is required to solve the stated problem
> which is how I was thinking of excessive.

That would be most unfortunate, I agree.

> I agree that the general idea of allowing users to choose to include or
> exclude comments is worth discussion along the same lines of large objects
> and privileges.

Good, then we can at least consider this particular feature as being one
we're generally interested in and move forward with it, even if we also,
perhaps, come up with a better solution to the specific issue at hand.

> > > > CREATE EXTENSION IF NOT EXISTS plpgsql ... COMMENT blah;
> > >
> > > ​A less verbose way to add comments to objects would be nice but we have
> > an
> > > immediate problem that we either need to solve or document a best
> > practice
> > > for.
> >
> > The above would be a solution to the immediate problem in as much as
> > adding COMMENT IF NOT EXISTS would be.
> 
> ​I believe it would take a lot longer, possibly even until 12, to get the
> linked comment feature committed compared ​to committing COMMENT IF NOT
> EXISTS or some variation (or putting in a hack for that matter).

Perhaps, but I'm not convinced that such speculation really helps move
us forward.

> > > COMMENT IF NOT EXISTS has been brought up but it doesn't actually map to
> > > what seems to me is the underlying problem...that people don't want a
> > > non-functional (usually...) aspect preventing successful restoration.
> >
> > I tend to disagree with this characterization- I'm of the opinion that
> > people are, rightly, confused as to why we bother to try and add a
> > COMMENT to an object which we didn't actually end up creating (as it
> > already existed), and then throw an error on it to boot.
> 
> My characterization does actually describe the current system though.

I'm not entirely sure what I was getting at above, to be honest, but I
believe we're generally on the same page here.  I certainly agree that
users don't expect a pg_dump to not be able to be successfully restored.
What I may have been getting at is simply that it's not about a lack of
COMMENT IF NOT EXISTS, in an ideal world, but perhaps that's what we
need to solve this particular issue, for now.

> While I won't doubt that people do hold your belief it is an underlying
> mis-understanding as to how PostgreSQL works since comments aren't, as you
> say below, actual attributes but rather objects in their own right.  I
> would love to have someone solve the systemic problem here.  But the actual
> complaint can probably be addressed without it.

Right, I tend to follow your line of thought here.

> >   Were pg_dump a
> > bit more intelligent of an application, it would realize that once the
> > CREATE ... IF NOT EXISTS returned a notice saying "this thing already
> > existed" that it would realize that it shouldn't try to adjust the
> > attributes of that object, as it was already existing.
> 
> ​pg_dump isn't in play during the restore which is where the error occurs.

Ah, but pg_dump could potentially dump out more complicated logic than
it does today.  We currently presume that there is never any need to
reason about the results of a prior command before executing the next in
pg_dump's output.  In some ways, it's rather impressive that we've
gotten this far with that assumption, but ensuring that is the case
means that our users are also able to rely on that and write simple
scripts which can be rerun to reset the database to a specific state.

> During restore you have pg_restore+psql - and having cross-statement logic
> in those applications is likely a non-starter. 

It would certainly be a very large shift from what we generate today. :)

> That is ultimately the
> problem here, 

Re: [HACKERS] Patch: Add --no-comments to skip COMMENTs with pg_dump

2017-05-30 Thread David G. Johnston
Stephen,

On Tue, May 30, 2017 at 8:41 PM, Stephen Frost  wrote:

> David,
>
> * David G. Johnston (david.g.johns...@gmail.com) wrote:
> > On Fri, May 26, 2017 at 7:47 AM, Stephen Frost 
> wrote:
> > > * Robins Tharakan (thara...@gmail.com) wrote:
> > > > Attached is a patch adds a --no-comments argument to pg_dump to skip
> > > > generation of COMMENT statements when generating a backup. This is
> > > crucial
> > > > for non-superusers to restore a database backup in a Single
> Transaction.
> > > > Currently, this requires one to remove COMMENTs via scripts, which is
> > > > inelegant at best.
> > >
> > > Having --no-comments seems generally useful to me, in any case.
> >
> > I​t smacks of being excessive to me.
> > ​
>
> I have a hard time having an issue with an option that exists to exclude
> a particular type of object from being in the dump.
>

​Excessive with respect to the problem at hand.  A single comment in the
dump is unable to be restored.  Because of that we are going to require
people to omit every comment in their database.  The loss of all that
information is in excess of what is required to solve the stated problem
which is how I was thinking of excessive.

I agree that the general idea of allowing users to choose to include or
exclude comments is worth discussion along the same lines of large objects
and privileges.


> > > CREATE EXTENSION IF NOT EXISTS plpgsql ... COMMENT blah;
> >
> > ​A less verbose way to add comments to objects would be nice but we have
> an
> > immediate problem that we either need to solve or document a best
> practice
> > for.
>
> The above would be a solution to the immediate problem in as much as
> adding COMMENT IF NOT EXISTS would be.
>

​I believe it would take a lot longer, possibly even until 12, to get the
linked comment feature committed compared ​to committing COMMENT IF NOT
EXISTS or some variation (or putting in a hack for that matter).


> > COMMENT IF NOT EXISTS has been brought up but it doesn't actually map to
> > what seems to me is the underlying problem...that people don't want a
> > non-functional (usually...) aspect preventing successful restoration.
>
> I tend to disagree with this characterization- I'm of the opinion that
> people are, rightly, confused as to why we bother to try and add a
> COMMENT to an object which we didn't actually end up creating (as it
> already existed), and then throw an error on it to boot.


My characterization does actually describe the current system though.
While I won't doubt that people do hold your belief it is an underlying
mis-understanding as to how PostgreSQL works since comments aren't, as you
say below, actual attributes but rather objects in their own right.  I
would love to have someone solve the systemic problem here.  But the actual
complaint can probably be addressed without it.


>   Were pg_dump a
> bit more intelligent of an application, it would realize that once the
> CREATE ... IF NOT EXISTS returned a notice saying "this thing already
> existed" that it would realize that it shouldn't try to adjust the
> attributes of that object, as it was already existing.


​pg_dump isn't in play during the restore which is where the error occurs.

During restore you have pg_restore+psql - and having cross-statement logic
in those applications is likely a non-starter. That is ultimately the
problem here, and which is indeed fixed by the outstanding proposal of
embedding COMMENT within the CREATE/ALTER object commands.  But today,
comments are independent objects and solving the problem within that domain
will probably prove easier than changing how the system treats comments.


> > COMMENT ON object TRY 'text'  -- i.e., replace the word IS with TRY
>
> Perhaps you could elaborate as to how this is different from IF NOT
> EXISTS?
>
>
​If the comment on plpgsql were removed for some reason the COMMENT ON IF
NOT EXISTS would fire and then it would fail due to permissions.  With
"TRY" whether the comment (or object for that matter) exists or not the new
comment would be attempted and if the permission failure kicked in it
wouldn't care.​


>  If the
> > affected users cannot make that work then maybe we should just remove the
> > comment from the extension.
>
> Removing the comment as a way to deal with our deficiency in this area
> strikes me as akin to adding planner hints.
>

​Maybe, but the proposal you are supporting has been around for years, with
people generally in favor of it, and hasn't happened yet.  At some point
I'd rather hold my nose and fix the problem myself than wait for the
professional to arrive and do it right.  Any, hey, we've had multiple
planner hints since 8.4 ;)

David J.


Re: [HACKERS] Patch: Add --no-comments to skip COMMENTs with pg_dump

2017-05-30 Thread Stephen Frost
David,

* David G. Johnston (david.g.johns...@gmail.com) wrote:
> On Fri, May 26, 2017 at 7:47 AM, Stephen Frost  wrote:
> > * Robins Tharakan (thara...@gmail.com) wrote:
> > > Attached is a patch adds a --no-comments argument to pg_dump to skip
> > > generation of COMMENT statements when generating a backup. This is
> > crucial
> > > for non-superusers to restore a database backup in a Single Transaction.
> > > Currently, this requires one to remove COMMENTs via scripts, which is
> > > inelegant at best.
> >
> > Having --no-comments seems generally useful to me, in any case.
> 
> I​t smacks of being excessive to me.
> ​

I have a hard time having an issue with an option that exists to exclude
a particular type of object from being in the dump.  A user who never
uses large objects/blobs might feel the same way about "--no-blobs", or
a user who never uses ACLs wondering why we have a "--no-privileges".
In the end, these are also possible components that users may wish to
have for their own reasons.

What I, certainly, agree isn't ideal is requiring users to use such an
option to generate a database-level dump file (assuming they have access
to more-or-less all database objects) which can be restored as a
non-superuser, that's why I was a bit hesitant about this particular
solution to the overall problem.

I do agree that if there is simply no use-case, ever, for wishing to
strip comments from a database then it might be excessive to provide
such an option, but I tend to feel that there is a reasonable, general,
use-case for the option.

> > CREATE EXTENSION IF NOT EXISTS plpgsql ... COMMENT blah;
> 
> ​A less verbose way to add comments to objects would be nice but we have an
> immediate problem that we either need to solve or document a best practice
> for.

The above would be a solution to the immediate problem in as much as
adding COMMENT IF NOT EXISTS would be.

> COMMENT IF NOT EXISTS has been brought up but it doesn't actually map to
> what seems to me is the underlying problem...that people don't want a
> non-functional (usually...) aspect preventing successful restoration.

I tend to disagree with this characterization- I'm of the opinion that
people are, rightly, confused as to why we bother to try and add a
COMMENT to an object which we didn't actually end up creating (as it
already existed), and then throw an error on it to boot.  Were pg_dump a
bit more intelligent of an application, it would realize that once the
CREATE ... IF NOT EXISTS returned a notice saying "this thing already
existed" that it would realize that it shouldn't try to adjust the
attributes of that object, as it was already existing.  That, however,
would preclude the ability of pg_dump to produce a text file used for
restore, unless we started to write into that text file DO blocks, which
I doubt would go over very well.

> COMMENT ON object TRY 'text'  -- i.e., replace the word IS with TRY

I'm not sure that I see why we should invent wholly new syntax for
something which we already have in IF NOT EXISTS, or why this should
really be viewed or considered any differently from the IF NOT EXISTS
syntax.

Perhaps you could elaborate as to how this is different from IF NOT
EXISTS?

> If the attempt to comment fails for any reason log a warning (maybe) but
> otherwise ignore the result and continue on without invoking an error.

In the IF NOT EXISTS case we log a NOTICE, which seems like it's what
would be appropriate here also, again begging the question of it this is
really different from IF NOT EXISTS in a meaningful way.

> One suggestion I've seen is to simply "COMMENT ON EXTENSION plpgsql IS
> NULL;"  If that works in the scenarios people are currently dealing with
> then I'd say we should advise that such an action be taken for those whom
> wish to generate dumps that can be loaded by non-super-users.  If the
> affected users cannot make that work then maybe we should just remove the
> comment from the extension.  People can lookup "plpgsql" in the docs easily
> enough and "PL/pgSQL procedural language" doesn't do anything more than
> expand the acronym.

Removing the comment as a way to deal with our deficiency in this area
strikes me as akin to adding planner hints.

Thanks!

Stephen


signature.asc
Description: Digital signature


Re: [HACKERS] Patch: Add --no-comments to skip COMMENTs with pg_dump

2017-05-30 Thread David G. Johnston
On Fri, May 26, 2017 at 7:47 AM, Stephen Frost  wrote:

> Greetings,
>
> * Robins Tharakan (thara...@gmail.com) wrote:
> > Attached is a patch adds a --no-comments argument to pg_dump to skip
> > generation of COMMENT statements when generating a backup. This is
> crucial
> > for non-superusers to restore a database backup in a Single Transaction.
> > Currently, this requires one to remove COMMENTs via scripts, which is
> > inelegant at best.
>
> Having --no-comments seems generally useful to me, in any case.
>

I​t smacks of being excessive to me.
​

> CREATE EXTENSION IF NOT EXISTS plpgsql ... COMMENT blah;
>

​A less verbose way to add comments to objects would be nice but we have an
immediate problem that we either need to solve or document a best practice
for.

COMMENT IF NOT EXISTS has been brought up but it doesn't actually map to
what seems to me is the underlying problem...that people don't want a
non-functional (usually...) aspect preventing successful restoration.

COMMENT ON object TRY 'text'  -- i.e., replace the word IS with TRY

If the attempt to comment fails for any reason log a warning (maybe) but
otherwise ignore the result and continue on without invoking an error.

One suggestion I've seen is to simply "COMMENT ON EXTENSION plpgsql IS
NULL;"  If that works in the scenarios people are currently dealing with
then I'd say we should advise that such an action be taken for those whom
wish to generate dumps that can be loaded by non-super-users.  If the
affected users cannot make that work then maybe we should just remove the
comment from the extension.  People can lookup "plpgsql" in the docs easily
enough and "PL/pgSQL procedural language" doesn't do anything more than
expand the acronym.

David J.


Re: [HACKERS] Patch: Add --no-comments to skip COMMENTs with pg_dump

2017-05-26 Thread Stephen Frost
Greetings,

* Robins Tharakan (thara...@gmail.com) wrote:
> Attached is a patch adds a --no-comments argument to pg_dump to skip
> generation of COMMENT statements when generating a backup. This is crucial
> for non-superusers to restore a database backup in a Single Transaction.
> Currently, this requires one to remove COMMENTs via scripts, which is
> inelegant at best.

Having --no-comments seems generally useful to me, in any case.

> A similar discussion had taken place earlier [1], however that stopped
> (with a Todo entry) since it required more work at the backend.

Well, that was one possible solution (COMMENT IF NOT EXISTS).  That does
seem like it'd be nice too, though what I think we really want here is
something like:

CREATE EXTENSION IF NOT EXISTS plpgsql ... COMMENT blah;

That general capability has been asked for and discussed before, but
it's complicated because we support comments on lots of objects and
doesn't address other possible issues with this approach (comments
aren't the only things that could exist on plpgsql, and I can't see us
having a way to get every component included in a single command...).

That leads to an interesting thought which we could consider, which
would be represented in some crazy syntax such as:

CREATE EXTENSION IF NOT EXISTS plpgsql ... (
  COMMENT xyz;
  GRANT USAGE ON EXTENSION plpgsql whatever;
);

> This patch provides a stop-gap solution until then. If the feedback is to
> tackle this is by filtering comments for specific DB objects, an argument
> name (for e.g. --no-extension-comments or something) would help and I could
> submit a revised patch.

That seems like it might be a bit too specific.

> Alternatively, if there are no objections, I could submit this to the
> Commitfest.

Yes, please add it to the commitfest.

Thanks!

Stephen


signature.asc
Description: Digital signature


[HACKERS] Patch: Add --no-comments to skip COMMENTs with pg_dump

2017-05-26 Thread Robins Tharakan
Hi,

Attached is a patch adds a --no-comments argument to pg_dump to skip
generation of COMMENT statements when generating a backup. This is crucial
for non-superusers to restore a database backup in a Single Transaction.
Currently, this requires one to remove COMMENTs via scripts, which is
inelegant at best.

A similar discussion had taken place earlier [1], however that stopped
(with a Todo entry) since it required more work at the backend.

This patch provides a stop-gap solution until then. If the feedback is to
tackle this is by filtering comments for specific DB objects, an argument
name (for e.g. --no-extension-comments or something) would help and I could
submit a revised patch.

Alternatively, if there are no objections, I could submit this to the
Commitfest.

References:
[1] https://www.postgresql.org/message-id/1420.1397099637%40sss.pgh.pa.us

-
robins
​​


pgdump_nocomments_v1.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers