Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

[Bossart, Nathan]

On 5/11/17, 6:32 PM, "Tom Lane"  wrote:
> You probably won't get much in the near term, because we're in
> stabilize-the-release mode not develop-new-features mode.
> Please add your patch to the pending commitfest
> https://commitfest.postgresql.org/14/
> so that we remember to look at it when the time comes.

Understood.  I’ve added it to the commitfest.

Nathan


-- 
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] [Proposal] Allow users to specify multiple tables in VACUUM commands

[Bossart, Nathan]

On 5/11/17, 7:20 PM, "Michael Paquier"  wrote:
> Browsing the code

Thanks for taking a look.

> It seems to me that you don't need those extra square brackets around
> the column list. Same remark for vacuum.sgml.

I’ll remove them.  My intent was to ensure that we didn’t accidentally suggest 
that statements like “VACUUM , foo, bar;” were accepted, but the extra brackets 
don’t seem to fix that, and I don’t foresee much confusion anyway.  

> In short for all that, it is enough to have "[, ... ]" to document
> that a list is accepted.

That makes sense, I’ll fix it.

> It seems to me that it would have been less invasive to loop through
> vacuum() for each relation. Do you foresee advantages in allowing
> vacuum() to handle multiple? I am not sure if is is worth complicating
> the current logic more considering that you have as well so extra
> logic to carry on option values.

That was the approach I first prototyped.  The main disadvantage that I found 
was that the command wouldn’t fail-fast if one of the tables or columns didn’t 
exist, and I thought that it might be frustrating to encounter such an error in 
the middle of vacuuming several large tables.  It’s easy enough to change the 
logic to emit a warning and simply move on to the next table, but that seemed 
like it could be easily missed among the rest of the vacuum log statements 
(especially with the verbose option specified).  What are your thoughts on this?

In the spirit of simplifying things a bit, I do think it is possible to 
eliminate one of the new node types, since the fields for each are almost 
identical.

> + * used for error messages.  In the case that relid is valid, rels
> + * must have exactly one element corresponding to the specified relid.
> s/rels/relations/ as variable name?

Agreed, that seems nicer.

Nathan



-- 
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] [Proposal] Allow users to specify multiple tables in VACUUM commands

[Bossart, Nathan]

On 5/10/17, 8:10 PM, "Masahiko Sawada"  wrote:
> I agree to report per-table information. Especially In case of one of
> tables specified failed during vacuuming, I think we should report at
> least information of tables that is done successfully so far.

+1

I believe you already get all per-table information when you do not specify a 
table name (“VACUUM VERBOSE;”), so I would expect to get this for free as long 
as we are building this into the existing ExecVacuum(…) and vacuum(…) functions.

Nathan


-- 
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] [Proposal] Allow users to specify multiple tables in VACUUM commands

[Bossart, Nathan]

On 5/16/17, 11:21 PM, "Michael Paquier" <michael.paqu...@gmail.com> wrote:
> On Wed, May 17, 2017 at 7:56 AM, Bossart, Nathan <bossa...@amazon.com> wrote:
>> I think this issue already exists, as this comment in get_rel_oids(…) seems 
>> to indicate:
>>
>> /*
>>  * Since we don't take a lock here, the relation might be gone, or the
>>  * RangeVar might no longer refer to the OID we look up here.  In the
>>  * former case, VACUUM will do nothing; in the latter case, it will
>>  * process the OID we looked up here, rather than the new one. Neither
>>  * is ideal, but there's little practical alternative, since we're
>>  * going to commit this transaction and begin a new one between now
>>  * and then.
>>  */
>>  relid = RangeVarGetRelid(vacrel, NoLock, false);
>>
>> With the patch applied, I believe this statement still holds true.  So if 
>> the relation disappears before we get to vacuum_rel(…), we will simply skip 
>> it and move on to the next one.  The vacuum_rel(…) code provides a WARNING 
>> in many cases (e.g. the user does not have privileges to VACUUM the table), 
>> but we seem to silently skip the table when it disappears before the call to 
>> vacuum_rel(…).
>
> Yes, that's the bits using try_relation_open() which returns NULL if
> the relation is gone. I don't think that we want VACUUM to be noisy
> about that when running it on a database.

Agreed.

>> If we added a WARNING to the effect of “skipping vacuum of  — 
>> relation no longer exists” for this case, I think what you are suggesting 
>> would be satisfied.
>
> We would do no favor by reporting nothing to the user. Without any
> information, the user triggering a manual vacuum may believe that the
> relation has been vacuumed as it was listed but that would not be the
> case.

Agreed.  Unfortunately, I think this is already possible when you specify a 
table to be vacuumed.

>> However, ANALYZE has a slight caveat.  While analyze_rel(…) silently skips 
>> the relation if it no longer exists like vacuum_rel(…) does, we do not 
>> pre-validate the columns list at all.  So, in an ANALYZE statement with 
>> multiple tables and columns specified, it’ll only fail once we get to the 
>> undefined column.  To fix this, we could add a check for the column lists 
>> near get_rel_oids(…) and adjust do_analyze_rel(…) to emit a WARNING and skip 
>> any columns that vanish in the meantime.
>>
>> Does this seem like a sane approach?
>>
>>   1. Emit WARNING when skipping if relation disappears before we get to it.
>>   2. Early in vacuum(…), check that the specified columns exist.
>
> And issue an ERROR, right?

Correct.  This means that both the relations and columns specified would cause 
an ERROR if they do not exist and a WARNING if they disappear before we can 
actually process them.

> +   RelationAndColumns *relinfo = (RelationAndColumns *)
> lfirst(relation);
> +   int per_table_opts = options | relinfo->options;  /*
> VACOPT_ANALYZE may be set per-table */
> +   ListCell *oid;
> I have just noticed this bit in your patch. So you can basically do
> something like that:
> VACUUM (ANALYZE) foo, (FULL) bar;
> Do we really want to go down to this level of control? I would keep
> things a bit more restricted as users may be confused by the different
> lock levels involved by each operation, and make use of the same
> options for all the relations listed. Opinions from others is welcome.

I agree with you here, too.  I stopped short of allowing customers to 
explicitly provide per-table options, so the example you provided wouldn’t work 
here.  This is more applicable for something like the following:

VACUUM (FREEZE, VERBOSE) foo, bar (a);

In this case, the FREEZE and VERBOSE options are used for both tables.  
However, we have a column list specified for ‘bar’, and the ANALYZE option is 
implied when we specify a column list.  So when we process ‘bar’, we need to 
apply the ANALYZE option, but we do not need it for ‘foo’.  For now, that is 
all that this per-table options variable is used for.

Nathan


-- 
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] [Proposal] Allow users to specify multiple tables in VACUUM commands

[Bossart, Nathan]

Great, I’ll keep working on this patch and will update this thread with a more 
complete implementation.

Nathan


-- 
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] [Proposal] Allow users to specify multiple tables in VACUUM commands

[Bossart, Nathan]

On 5/18/17, 8:03 PM, "Tom Lane" <t...@sss.pgh.pa.us> wrote:
>”Bossart, Nathan" <bossa...@amazon.com> writes:
>> On 5/18/17, 6:12 PM, "Michael Paquier" <michael.paqu...@gmail.com> wrote:
>>> Fine for me as well. I would suggest to split the patch into two parts
>>> to ease review then:
>>> - Rework this error handling for one relation.
>>> - The main patch.
>>
>> I’d be happy to do so, but I think part one would be pretty small, and 
>> almost all of the same code needs to be changed in the main patch anyway.  I 
>> do not foresee a huge impact on review-ability either way.  If others 
>> disagree, I can split it up.
>
>Yeah, I'm dubious that that's really necessary.  If the change proves
>bigger than you're anticipating, maybe it's worth a two-step approach,
>but I share your feeling that it probably isn’t.

Just in case it was missed among the discussion, I’d like to point out that v5 
of the patch includes the “ERROR if ANALYZE not specified” change.

Nathan


-- 
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] [Proposal] Allow users to specify multiple tables in VACUUM commands

[Bossart, Nathan]

On 5/18/17, 6:12 PM, "Michael Paquier"  wrote:
> Fine for me as well. I would suggest to split the patch into two parts
> to ease review then:
> - Rework this error handling for one relation.
> - The main patch.

I’d be happy to do so, but I think part one would be pretty small, and almost 
all of the same code needs to be changed in the main patch anyway.  I do not 
foresee a huge impact on review-ability either way.  If others disagree, I can 
split it up.

Nathan


-- 
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] Support to COMMENT ON DATABASE CURRENT_DATABASE

[Bossart, Nathan]

A few general comments.

While this patch applies, I am still seeing some whitespace errors:

  comment_on_current_database_no_pgdump_v4.1.patch:488: trailing whitespace.
ColId
  comment_on_current_database_no_pgdump_v4.1.patch:490: trailing whitespace.
| CURRENT_DATABASE
  comment_on_current_database_no_pgdump_v4.1.patch:491: trailing whitespace.
{
  comment_on_current_database_no_pgdump_v4.1.patch:501: trailing whitespace.
ColId
  comment_on_current_database_no_pgdump_v4.1.patch:502: trailing whitespace.
{
  warning: squelched 9 whitespace errors
  warning: 14 lines add whitespace errors.

It looks like the 'dbname' test is currently failing when you run
'make check-world'.  The Travis CI build log [1] shows the details
of the failure.  From a brief glance, I would guess that some of
the queries are returning unexpected databases that are created in
other tests.

Also, I think this change will require updates to the
documentation.

+   if (dbspecname->dbnametype == DBSPEC_CURRENT_DATABASE )
+   dbname = get_database_name(MyDatabaseId);
+   else
+   dbname = dbspecname->dbname;

This pattern is repeated in the patch several times.  It looks like
the end goal of these code blocks is either to get the database
name or the database OID, so perhaps we should have
get_dbspec_name() and get_dbspec_oid() helper functions (like
get_rolespec_oid() for RoleSpec nodes).

+static bool
+_equalDatabaseSpec(const DBSpecName *a, const DBSpecName *b)
+{
+   COMPARE_SCALAR_FIELD(dbnametype);
+   COMPARE_STRING_FIELD(dbname);
+   COMPARE_LOCATION_FIELD(location);
+
+   return true;
+}

There are some inconsistencies in the naming strategy.  For
example, this function is called _equalDatabaseSpec(), but the
struct is DBSpecName.  I would suggest calling it DatabaseSpec or
DbSpec throughout the patch.

Nathan

[1] https://travis-ci.org/postgresql-cfbot/postgresql/builds/275747367


-- 
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] Rethinking autovacuum.c memory handling

[Bossart, Nathan]

On 9/23/17, 5:27 AM, "Michael Paquier"  wrote:
>On Sat, Sep 23, 2017 at 6:09 AM, Tom Lane  wrote:
>> I notice that autovacuum.c calls autovacuum_do_vac_analyze, and
>> thereby vacuum(), in TopTransactionContext.  This doesn't seem
>> like a terribly great idea, because it doesn't correspond to what
>> happens during a manually-invoked vacuum.
>
> Indeed, the inconsistency is not good here.
>
>> What I think we should do instead is invoke autovacuum_do_vac_analyze
>> in the PortalContext that do_autovacuum has created, which we already
>> have a mechanism to reset once per table processed in do_autovacuum.
>>
>> The attached patch does that, and also modifies perform_work_item()
>> to use the same approach.  Right now perform_work_item() has a
>> copied-and-pasted MemoryContextResetAndDeleteChildren(PortalContext)
>> call in its error recovery path, but that seems a bit out of place
>> given that perform_work_item() isn't using PortalContext otherwise.
>
> I have spent some time looking at your patch and testing it. This
> looks sane. A small comment that I have would be to add an assertion
> at the top of perform_work_item to be sure that it is called in the
> memory context of AutovacMemCxt.

This looks reasonable to me as well.  I haven't noticed any issues after
a couple hours of pgbench with aggressive autovacuum settings, either.

Nathan


-- 
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] Rethinking autovacuum.c memory handling

[Bossart, Nathan]

On 9/23/17, 12:36 PM, "Tom Lane" <t...@sss.pgh.pa.us> wrote:
>"Bossart, Nathan" <bossa...@amazon.com> writes:
>> This looks reasonable to me as well.  I haven't noticed any issues after
>> a couple hours of pgbench with aggressive autovacuum settings, either.
>
> Thanks for looking.  As I'm sure you realize, what motivated that was
> not liking the switch into AutovacMemCxt that you'd added in
> autovacuum_do_vac_analyze ... with this patch, we can drop that.

Yup.  I’ll go ahead and update that patch now.

Nathan



-- 
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] [Proposal] Allow users to specify multiple tables in VACUUM commands

[Bossart, Nathan]

On 9/20/17, 2:29 PM, "Tom Lane"  wrote:
> I started to look at this...

Thanks for taking a look.

> I think that the way this ought to work is you process the VacuumRelation
> structs purely sequentially, each in its own transaction, so you don't
> need leaps of faith about what to do if a relation changes between the
> first time you look at it and when you actually come to process it.

How might this work for VACUUM statements with no tables specified?  It
seems like we must either handle the case when the relation changes before
it is processed or hold a lock for the duration of the vacuuming.

> The idea of "fast failing" because some later VacuumRelation struct might
> contain an error seems like useless complication, both in terms of the
> implementation and the user-visible behavior.

I agree that the patch might be simpler without this, but the user-visible
behavior is the reason I had included it.  In short, my goal was to avoid
errors halfway through a long-running VACUUM statement because the user
misspelled a relation/column name or the relation/column was dropped.
It's true that the tests become mostly pointless if the relations or
columns change before they are processed, but this seems like it would be
a rarer issue in typical use cases.

I'll continue to look into switching to a more sequential approach and
eliminating the leaps of faith.

> As for the other patch, I wonder if it might not be better to
> silently ignore duplicate column names.  But in either case, we don't
> need a pre-check, IMO.  I'd just leave it to the lookup loop in
> do_analyze_rel to notice if it's looked up the same column number
> twice.  That would be more likely to lead to a back-patchable fix,
> too.  0002 as it stands is useless as a back-patch basis, since it
> proposes modifying code that doesn't even exist in the back branches.

I'd be happy to write something up that is more feasibly back-patched.

Nathan


-- 
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] Additional logging for VACUUM and ANALYZE

[Bossart, Nathan]

On 10/5/17, 12:29 AM, "Michael Paquier" <michael.paqu...@gmail.com> wrote:
> On Thu, Oct 5, 2017 at 1:23 AM, Bossart, Nathan <bossa...@amazon.com> wrote:
>> Presently, there are a few edge cases in vacuum_rel() and analyze_rel() that 
>> I
>> believe do not have sufficient logging.  This was discussed a bit in the
>> vacuum-multiple-relations thread [0], but it was ultimately decided that any
>> logging changes should be proposed separately.
>
> I think that I agree with that, especially now with VACUUM allowing
> multiple relations. The discussion then would be how much logging we
> want. WARNING looks adapted per the discussions we had on the other
> thread as manual VACUUMs can now involve much more relations, even
> with partitioned tables. More opinions would be welcome.

Thanks for taking a look.

>> and a test that exercises a bit of this functionality.
>
> My take on those test would be to not include them. This is a lot just
> to test two logging lines where the relation has been dropped.

Yeah, I'm not terribly concerned about those tests.

>> If this change were to be considered for back-patching, we would likely want 
>> to
>> also apply Michael's RangeVar fix for partitioned tables to 10 [1].  Without
>> this change, log messages for unspecified partitions will be emitted with the
>> parent's RangeVar information.
>
> Well, that's assuming that we begin logging some information for
> manual VACUUMs using the specified RangeVar, something that does not
> happen at the top of upstream REL_10_STABLE, but can happen if we were
> to include the patch you are proposing on this thread for
> REL_10_STABLE. But the latter is not going to happen. Or did you patch
> your version of v10 to do so in your stuff? For v10 the ship has
> already sailed, so I think that it would be better to just let it go,
> and rely on v11 which has added all the facility we wanted.

I agree.  I didn't mean to suggest that it should be back-patched, just
that v10 has a small quirk that needs to be handled if others feel
differently.

Nathan


-- 
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] [Proposal] Allow users to specify multiple tables in VACUUM commands

[Bossart, Nathan]

On 8/30/17, 5:37 PM, "Michael Paquier"  wrote:
> Yeah... Each approach has its cost and its advantages. It may be
> better to wait for more opinions, no many people have complained yet
> that for example a list of columns using twice the same one fails.

Sounds good to me.

> +VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] [  class="PARAMETER">table_name ] [, ...]
> I just noticed that... But regarding the docs, I think that you have
> misplaced the position of "[, ...]", which should be inside the
> table_name portion in the case of what I quote here, no?

I think that's what I had initially, but it was changed somewhere along
the line.  It is a little more complicated for the versions that accept
column lists.

VACUUM ... ANALYZE [ [ table_name [ (column_name [, ...] ) ] ] [, ...] ]

ISTM that we need the extra brackets here to clarify that the table and
column list combination is what can be provided in a list.  Does that
make sense?  Or do you think we can omit the outermost brackets here?

> +VacuumRelation *
> +makeVacuumRelation(RangeVar *relation, List *va_cols, Oid oid)
> +{
> +   VacuumRelation *vacrel = makeNode(VacuumRelation);
> +   vacrel->relation = relation;
> +   vacrel->va_cols = va_cols;
> +   vacrel->oid = oid;
> +   return vacrel;
> +}
> Perhaps in makefuncs.c instead of vacuum.c? That's usually the place
> used for node constructions like that.

Ah, yes.  That is a much better place.  I'll make this change.

Nathan


-- 
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] [Proposal] Allow users to specify multiple tables in VACUUM commands

[Bossart, Nathan]

On 9/4/17, 8:16 PM, "Michael Paquier"  wrote:
> So vacuum_multiple_tables_v14.patch is good for a committer in my
> opinion. With this patch, if the same relation is specified multiple
> times, then it gets vacuum'ed that many times. Using the same column
> multi-times results in an error as on HEAD, but that's not a new
> problem with this patch.

Thanks!

> So I would tend to think that the same column specified multiple times
> should cause an error, and that we could let VACUUM run work N times
> on a relation if it is specified this much. This feels more natural,
> at least to me, and it keeps the code simple.

I think that is a reasonable approach.  Another option I was thinking
about was to de-duplicate only the individual column lists.  This
alternative approach might be a bit more user-friendly, but I am
beginning to agree with you that perhaps we should not try to infer
the intent of the user in these "duplicate" scenarios.

I'll work on converting the existing de-duplication patch into
something more like what you suggested.

Nathan


-- 
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] [Proposal] Allow users to specify multiple tables in VACUUM commands

[Bossart, Nathan]

On 9/4/17, 10:32 PM, "Simon Riggs"  wrote:
> ISTM there is no difference between
>   VACUUM a, b
> and
>   VACUUM a; VACUUM b;
>
> If we want to keep the code simple we must surely consider whether the
> patch has any utility.

Yes, this is true, but I think the convenience factor is a bit
understated with that example.  For example, if you need to manually
cleanup several tables for XID purposes,
VACUUM FREEZE VERBOSE table1;
VACUUM FREEZE VERBOSE table2;
VACUUM FREEZE VERBOSE table3;
VACUUM FREEZE VERBOSE table4;
VACUUM FREEZE VERBOSE table5;
becomes
VACUUM FREEZE VERBOSE table1, table2, table3, table4, table5;

I would consider even this to be a relatively modest example compared
to the sorts of things users might do.

In addition, I'd argue that this feels like a natural extension of the
VACUUM command, one that I, like others much earlier in this thread,
was surprised to learn wasn't supported.

Nathan


-- 
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] [Proposal] Allow users to specify multiple tables in VACUUM commands

[Bossart, Nathan]

On 9/9/17, 7:28 AM, "Michael Paquier"  wrote:
> In the duplicate patch, it seems to me that you can save one lookup at
> the list of VacuumRelation items by checking for column duplicates
> after checking that all the columns are defined. If you put the
> duplicate check before closing the relation you can also use the
> schema name associated with the Relation.

I did this so that the ERROR prioritization would be enforced across all
relations.  For example:

VACUUM ANALYZE table1 (a, a), table2 (does_not_exist);

If I combine the 'for' loops to save a lookup, this example behaves
differently.  Instead of an ERROR for the nonexistent column, you would
hit an ERROR for the duplicate column in table1's list.  However, I do
not mind changing this.

> +   if (i == InvalidAttrNumber)
> +   ereport(ERROR,
> +   (errcode(ERRCODE_UNDEFINED_COLUMN),
> +errmsg("column \"%s\" of relation \"%s\" does not exist",
> +   col, RelationGetRelationName(rel;
> This could use the schema name unconditionally as you hold a Relation
> here, using RelationGetNamespace().

Sure, I think this is a good idea.  I'll make this change in the next
version of the patch.

> if (!onerel)
> +   {
> +   /*
> +* If one of the relations specified by the user has disappeared
> +* since we last looked it up, let them know so that they do not
> +* think it was actually analyzed.
> +*/
> +   if (!IsAutoVacuumWorkerProcess() && relation)
> +   ereport(WARNING,
> + (errmsg("skipping \"%s\" --- relation no longer exists",
> + relation->relname)));
> +
> return;
> +   }
> Hm. So if the relation with the defined OID is not found, then we'd
> use the RangeVar, but this one may not be set here:
> +   /*
> +* It is safe to leave everything except the OID empty here.
> +* Since no tables were specified in the VacuumStmt, we know
> +* we don't have any columns to keep track of.  Also, we do
> +* not need the RangeVar, because it is only used for error
> +* messaging when specific relations are chosen.
> +*/
> +   rel_oid = HeapTupleGetOid(tuple);
> +   relinfo = makeVacuumRelation(NULL, NIL, rel_oid);
> +   vacrels_tmp = lappend(vacrels_tmp, relinfo);
> So if the relation is analyzed but skipped, we would have no idea that
> it actually got skipped because there are no reports about it. That's
> not really user-friendly. I am wondering if we should not instead have
> analyze_rel also enforce the presence of a RangeVar, and adding an
> assert at the beginning of the function to undertline that, and also
> do the same for vacuum(). It would make things also consistent with
> vacuum() which now implies on HEAD that a RangeVar *must* be
> specified.

I agree that it is nice to see when relations are skipped, but I do not
know if the WARNING messages would provide much value for this
particular use case (i.e. 'VACUUM;').  If a user does not provide a list
of tables to VACUUM, they might not care too much about WARNINGs for
dropped tables.

> Are there opinions about back-patching the patch checking for
> duplicate columns? Stable branches now complain about an unhelpful
> error message.

I wouldn't mind drafting something up for the stable branches.

Nathan


-- 
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] Shaky coding for vacuuming partitioned relations

[Bossart, Nathan]

On 9/25/17, 6:51 PM, "Michael Paquier"  wrote:
>> +* Take a lock here for the relation lookup. If ANALYZE or 
>> VACUUM spawn
>> +* multiple transactions, the lock taken here will be gone 
>> once the
>> +* current transaction running commits, which could cause 
>> the relation
>> +* to be gone, or the RangeVar might not refer to the OID 
>> looked up here.
>>
>> I think this could be slightly misleading.  Perhaps it would be more
>> accurate to say that the lock will be gone any time vacuum() creates a new
>> transaction (either in vacuum_rel() or when use_own_xacts is true).
>
> The comment of the proposed patch matches as much as possible what is
> currently on HEAD, so I would still go with something close to that.

Sure.  This is just a minor point, and I could see the argument that your
phrasing is more concise, anyway.

Nathan


-- 
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] [Proposal] Allow users to specify multiple tables in VACUUM commands

[Bossart, Nathan]

On 9/25/17, 12:42 AM, "Michael Paquier"  wrote:
> +   if (!IsAutoVacuumWorkerProcess())
> +   ereport(WARNING,
> + (errmsg("skipping \"%s\" --- relation no longer exists",
> + relation->relname)));
> I like the use of WARNING here, but we could use as well a LOG to be
> consistent when a lock obtention is skipped.

It looks like the LOG statement is only emitted for autovacuum, so maybe
we should keep this at WARNING for consistency with the permission checks
below it.

> +* going to commit this transaction and begin a new one between 
> now
> +* and then.
> +*/
> +   relid = RangeVarGetRelid(relinfo->relation, NoLock, false);
> We will likely have to wait that the matters discussed in
> https://www.postgresql.org/message-id/25023.1506107...@sss.pgh.pa.us
> are settled.

Makes sense.

> +VACUUM FULL vactst, vactst, vactst, vactst;
> This is actually a waste of cycles.

I'll clean this up in v22.

Nathan


-- 
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] Enhancements to passwordcheck

[Bossart, Nathan]

On 9/25/17, 8:31 PM, "Michael Paquier"  wrote:
> Yes, I have developped a couple of years back a fork of passwordcheck
> which had much similar enhancements, so getting something more modular
> in upstream would be really welcome.

Awesome.

> On top of that I think that you should have a parameter that specifies
> a string full of special characters. For example I have been using
> things like !@#$%^&*()_+{}|<>?=. But you likely want to make that
> modular, people have their own set of character characters, and it is
> just something that could be compared with strchr(), still the default
> should be meaningful.

+1

>> passwordcheck.superuser_can_bypass
>
> Not sure that this one has much meaning. Superusers could easily
> unload the module.

True.  I can leave it out for now.

>> passwordcheck.force_new_password
>
> So this is to make sure that the new password is not the same as the
> old one? This would be better with the last N passwords, still this
> would require more facilities. I would discard this one as what you
> are proposing here is not modular enough IMO, and needs a separate
> feature set.

Fair enough.  I'll definitely start with a set of very non-controversial
parameters, as this will likely require rewriting most of the module.

>> I'd like to use this thread to gauge community interest in adding this
>> functionality to this module.  If there is interest, I'll add it to the next
>> commitfest.
>
> I think that's a good idea. Great to see that you are contributing
> back some stuff.

Thanks for the detailed feedback.

On 9/25/17, 8:37 PM, "Euler Taveira"  wrote:
>> passwordcheck.max_expiry_period
>>
> How would you enforce that? If the password expires, you can log in to
> change the password. If you let him/her to get in and change the
> password, you can't obligate the user to do that. You could send a
> message to remember that the password will expire but you can't
> enforce that (unless you make a change in the protocol).

My idea was to tie into the existing password expiration functionality
(VALID UNTIL in CREATE/ALTER ROLE statements).  I don't think this would
involve any changes to how password expiration works.  Instead, it would
just be a simple check to make sure the specified expiration date is not
too far in the future.

>> passwordcheck.force_new_password
>>
> Does it mean a password different from the old one? +1. It could be
> different from the last 3 passwords but we don't store a password
> history.

Yes.  As Michael pointed out, this might be better to do as a separate
effort since we'll almost certainly need to introduce a way to store
password history.

One interesting design challenge will be how to handle pre-hashed
passwords, since the number of checks we can do on those is pretty
limited.  I'm currently thinking of a parameter that can be used to
block, allow, or force pre-hashed passwords.  If we take that route,
perhaps we will also need to ensure that PostgreSQL fails to start when
invalid combinations are specified (e.g. pre-hashed passwords are forced
and min_password_length is nonzero).  Thoughts?

Also, I imagine we'll want to keep the cracklib and "password cannot
contain role name" features around, although perhaps these could become
configurable like the rest of the options.

Nathan


-- 
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] Enhancements to passwordcheck

[Bossart, Nathan]

On 9/26/17, 2:38 AM, "Albe Laurenz"  wrote:
> Nathan Bossart wrote:
 passwordcheck.force_new_password

>>> Does it mean a password different from the old one? +1. It could be
>>> different from the last 3 passwords but we don't store a password
>>> history.
>> 
>> Yes.  As Michael pointed out, this might be better to do as a separate
>> effort since we'll almost certainly need to introduce a way to store
>> password history.
>
> That increases the number of passwords stored on the server and
> consequently the damage when that list is stolen.
> Of course the old passwords are invalid, but if someone cracks them
> they could still try them on other systems the person uses.
>
> I think we should accept such a risk only if the benefits are clear, and
> my opinion has always been that if you forbid password reuse, people
> tend to come up with password generation schemes that are no better
> than the original passwords.

Right.  However, without this, they may not change the password at
all, which would make the expiry functionality less effective.  I
suppose there's not a great way to guard against these sorts of
password generation schemes without being able to judge the proposed
password against the previous password, too.

Perhaps the max_expiry_period parameter should be left out for now
as well.

>> One interesting design challenge will be how to handle pre-hashed
>> passwords, since the number of checks we can do on those is pretty
>> limited.  I'm currently thinking of a parameter that can be used to
>> block, allow, or force pre-hashed passwords.  If we take that route,
>> perhaps we will also need to ensure that PostgreSQL fails to start when
>> invalid combinations are specified (e.g. pre-hashed passwords are forced
>> and min_password_length is nonzero).  Thoughts?
>
> As was pointed out in the original discussion
> d960cb61b694cf459dcfb4b0128514c203937...@exadv11.host.magwien.gv.at
> the weak point of "passwordcheck" is that it does not work very well
> for encrypted passwords.
> The only saving grace is that you can at least check against
> username equals password.

Thanks for linking the original thread.  There are a lot of
interesting points.  I wonder if enhanced password checking in core
or contrib might be received differently with the introduction of
SCRAM authentication, since the weaknesses of MD5 were often cited.

> Disabling pre-hashed passwords in order to allow better password
> checks is a problem rather than a solution, because it exposes you
> to password theft of the clear-text password.  I think we shouldn't
> go there.
>
> The overall opinion in the above thread was that if you *really* care
> about security, you don't use database passwords, but external
> authentication with a centralized identity management system.
>
> So I think it is fine to extend "passwordcheck", but we shouldn't
> take it serious enough to reduce security elsewhere in order to
> improve the module.

I understand the points made here, but not allowing configurability
here really hinders the module's ability to enforce much of
anything.  However, I did say I wanted to avoid controversial
parameters, so I'll plan to count this one out as well.

This leaves us with the following proposed parameters for now:

passwordcheck.min_password_length
passwordcheck.min_uppercase_letters
passwordcheck.min_lowercase_letters
passwordcheck.min_numbers
passwordcheck.min_special_chars
passwordcheck.special_chars
passwordcheck.allow_username_in_password
passwordcheck.use_cracklib

Nathan


-- 
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] Use of RangeVar for partitioned tables in autovacuum

[Bossart, Nathan]

On 9/26/17, 9:28 PM, "Michael Paquier"  wrote:
> In conclusion, I think that the open item of $subject should be
> removed from the list, and we should try to get the multi-VACUUM patch
> in to cover any future problems. I'll do so if there are no
> objections.

If someone did want to add logging for vacuum_rel() and analyze_rel() in
v10 after your patch was applied, wouldn't the NULL RangeVars force us to
skip the new log statements for partitions?  I think we might instead
want to back-patch the VacuumRelation infrastructure so that we can
appropriately log for partitions.

However, I'm dubious that it is necessary to make such a big change so
close to release for hypothetical log statements. So, in the end, I agree
with you.

Nathan


-- 
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] Shaky coding for vacuuming partitioned relations

[Bossart, Nathan]

On 9/29/17, 11:18 AM, "Robert Haas"  wrote:
> I don't think I understand problem #2.  I think the concern is about
> reporting the proper relation name when VACUUM cascades from a
> partitioned table to its children and then some kind of concurrent DDL
> happens, but I don't see a clear explanation on the thread as to what
> exactly the failure scenario is, and I didn't see a problem in some
> simple tests I just ran.  Furthermore, it sounds like this might get
> fixed as part of committing the patch to allow VACUUM to mention
> multiple tables, which Tom has indicated he will handle.

Yes.  It looks like v10 is safe, and the vacuum-multiple-relations
patch should help prevent any future logging issues caused by this.

Discussion here: 
http://postgr.es/m/CAB7nPqRX1465FP%2Bameysxxt63tCQDDW6KvaTPMfkSxaT1TFGfw%40mail.gmail.com

Nathan


-- 
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] [Proposal] Allow users to specify multiple tables in VACUUM commands

[Bossart, Nathan]

On 9/28/17, 8:46 PM, "Michael Paquier" <michael.paqu...@gmail.com> wrote:
> On Fri, Sep 29, 2017 at 10:44 AM, Tom Lane <t...@sss.pgh.pa.us> wrote:
>> Michael Paquier <michael.paqu...@gmail.com> writes:
>>> On Fri, Sep 29, 2017 at 2:44 AM, Bossart, Nathan <bossa...@amazon.com> 
>>> wrote:
>>>> Alright, I've added logging for autovacuum in v23.  I ended up needing to
>>>> do a little restructuring to handle the case when the relation was skipped
>>>> because the lock could not be obtained.  While doing so, I became
>>>> convinced that LOG was probably the right level for autovacuum logs.
>>
>>> OK, of course let's not change the existing log levels. This could be
>>> always tuned later on depending on feedback from others. I can see
>>> that guc.c also uses elevel == 0 for some logic, so we could rely on
>>> that as you do.
>>
>> FWIW, I don't think this patch should be mucking with logging behavior
>> at all; that's not within its headline charter, and I doubt many people
>> are paying attention.  I propose to commit it without that.  If you feel
>> hot about changing the logging behavior, you can resubmit that as a new
>> patch in a new thread where it will get some visibility and debate on
>> its own merits.
>
> Okay. I am fine with that as well.

Sure, that seems reasonable to me.

Nathan


-- 
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] Shaky coding for vacuuming partitioned relations

[Bossart, Nathan]

On 9/24/17, 10:12 PM, "Michael Paquier"  wrote:
> Attached is a proposal of patch.

The patch seems reasonable to me, and I haven't encountered any issues in
my tests, even after applying the vacuum-multiple-relations patch on top
of it.

+* Take a lock here for the relation lookup. If ANALYZE or 
VACUUM spawn
+* multiple transactions, the lock taken here will be gone once 
the
+* current transaction running commits, which could cause the 
relation
+* to be gone, or the RangeVar might not refer to the OID 
looked up here.

I think this could be slightly misleading.  Perhaps it would be more
accurate to say that the lock will be gone any time vacuum() creates a new
transaction (either in vacuum_rel() or when use_own_xacts is true).

Nathan


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

[HACKERS] Enhancements to passwordcheck

[Bossart, Nathan]

Hi hackers,

Currently, the passwordcheck module provides a few basic checks to strengthen
passwords.  However, any configuration must be ready at compile time, and many
common password requirements cannot be enforced without creating a custom
version of this module.  I think there are a number of useful parameters that
could be added to enable common password restrictions, including the following
list, which is based on some asks from our customers:

passwordcheck.min_password_length
passwordcheck.min_uppercase_letters
passwordcheck.min_lowercase_letters
passwordcheck.min_numbers
passwordcheck.min_special_chars
passwordcheck.superuser_can_bypass
passwordcheck.max_expiry_period
passwordcheck.force_new_password

I'd like to use this thread to gauge community interest in adding this
functionality to this module.  If there is interest, I'll add it to the next
commitfest.

Nathan


-- 
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] [Proposal] Allow users to specify multiple tables in VACUUM commands

[Bossart, Nathan]

On 8/23/17, 11:59 PM, "Michael Paquier"  wrote:
> Robert, Amit and other folks working on extending the existing
> partitioning facility would be in better position to answer that, but
> I would think that we should have something as flexible as possible,
> and storing a list of relation OID in each VacuumRelation makes it
> harder to track the uniqueness of relations vacuumed. I agree that the
> concept of a partition with multiple parents induces a lot of
> problems. But the patch as proposed worries me as it complicates
> vacuum() with a double loop: one for each relation vacuumed, and one
> inside it with the list of OIDs. Modules calling vacuum() could also
> use flexibility, being able to analyze a custom list of columns for
> each relation has value as well.

I understand your concern, and I'll look into this for v9.  I think the
majority of this change will go into get_rel_oids(...).  Like you, I am
also curious to what the partitioning folks think.

> + * relations is a list of VacuumRelations to process.  If it is NIL, all
> + * relations in the database are processed.
> Typo here, VacuumRelation is singular.

I'll make this change in v9.

I should also note that the dedupe_relations(...) function needs another
small fix for column lists.  Since the lack of a column list means that we
should ANALYZE all columns, a duplicate table name with an empty column
list should effectively null out any other specified columns.  For example,
"ANALYZE table (a, b), table;" currently dedupes to the equivalent of
"ANALYZE table (a, b);" when it should dedupe to "ANALYZE table;".

Nathan


-- 
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] [Proposal] Allow users to specify multiple tables in VACUUM commands

[Bossart, Nathan]

On 9/3/17, 11:46 PM, "Michael Paquier"  wrote:
> I did not consider first that the list portion also needed to be
> modified, perhaps because I am not coding that myself... So now that
> it is becoming more complicated what about just using AutovacMemCxt?
> This would simplify the list of VacuumRelation entries and the
> RangeVar creation as well, and honestly this is ugly and there are no
> other similar patterns in the backend code:

+1

> This would become way more readable by using makeRangeVar() and the
> new makeVacuumRelation. As this is partly my fault that we are at this
> state, I am fine as well to remove this burden from you, Nathan, and
> fix that myself in a new version. And I don't want to step on your
> toes either :)

No worries, I can take care of it.  I appreciate your patience with all
of these reviews.

Nathan


-- 
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] [Proposal] Allow users to specify multiple tables in VACUUM commands

[Bossart, Nathan]

On 10/3/17, 5:59 PM, "Tom Lane"  wrote:
> I thought it would be a good idea to get this done before walking away
> from the commitfest and letting all this info get swapped out of my
> head.  So I've reviewed and pushed this.

Thanks!

> I took out most of the infrastructure you'd put in for constructing
> RangeVars for tables not explicitly named on the command line.  It
> was buggy (eg you can't assume a relcache entry will stick around)
> and I don't believe it's necessary.  I don't think that warnings
> should be issued for any tables not explicitly named.
>
> In any case, though, the extent to which we should add more warning
> or log output seems like a fit topic for a new thread and a separate
> patch.  Let's call this one done.

I'll look into submitting that to the next commitfest.

Nathan


-- 
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] Enhancements to passwordcheck

[Bossart, Nathan]

On 9/27/17, 7:41 AM, "Peter Eisentraut" <peter.eisentr...@2ndquadrant.com> 
wrote:
> On 9/25/17 23:10, Bossart, Nathan wrote:
>> One interesting design challenge will be how to handle pre-hashed
>> passwords, since the number of checks we can do on those is pretty
>> limited.  I'm currently thinking of a parameter that can be used to
>> block, allow, or force pre-hashed passwords.
>
> Pre-hashed passwords are the normal case.  You can't break that without
> making this module a net loss in security.

A strength in making this configurable is that you could use it to
enforce that passwords are pre-hashed.  But yes, blocking pre-
hashed passwords could be undesirable given an untrusted network or
server.

Nathan


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

[HACKERS] pg_upgrade to clusters with a different WAL segment size

[Bossart, Nathan]

Hi hackers,

The thread for the recent change to allow setting the WAL segment size at
initdb time [0] included a bit of discussion regarding pg_upgrade [1],
where it was suggested that relaxing an error check (presumably in
check_control_data()) might be enough to upgrade servers to a different WAL
segment size.

We've had success with our initial testing of upgrades to larger WAL
segment sizes, including post-upgrade pgbench runs.  Beyond adjusting
check_control_data(), it looks like the 'pg_resetwal -l' call in
copy_xact_xlog_xid() may need to be adjusted to ensure that the WAL
starting address is set to a valid value.

Allowing changes to the WAL segment size during pg_upgrade seems like a
nice way to avoid needing a dump and load, so I would like to propose
adding support for this.  I'd be happy to submit patches for this in the
next commitfest.

Nathan

[0] 
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=fc49e24fa69a15efacd5b8958115ed9c43c48f9a
[1] 
https://www.postgresql.org/message-id/20160826043926.pwkz45ksxpmfn4g6%40alap3.anarazel.de


-- 
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] Enhancements to passwordcheck

[Bossart, Nathan]

On 11/1/17, 9:33 PM, "Peter Eisentraut" <peter.eisentr...@2ndquadrant.com> 
wrote:
> On 9/25/17 14:04, Bossart, Nathan wrote:
>> I'd like to use this thread to gauge community interest in adding this
>> functionality to this module.  If there is interest, I'll add it to the next
>> commitfest.
>
> This thread has no patch, which is not really the intended use of the
> commit fest, so I'm closing the commit fest entry for now.

Sorry about that.  I'll reopen it when I have a patch to share.

Nathan


-- 
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] Support to COMMENT ON DATABASE CURRENT_DATABASE

[Bossart, Nathan]

On 10/5/17, 11:53 PM, "Jing Wang"  wrote:
> The patch has been updated according to Nathan's comments. 
> Thanks Nathan's review.

Thanks for the new versions of the patches.  I apologize for
the long delay for this new review.

It looks like the no-pgdump patch needs a rebase at this point.
I was able to apply the code portions with a 3-way merge, but
the documentation changes still did not apply.  I didn't have
any problems applying the pgdump patch.

+
+ 
+  The name of a database or keyword current_database. When using 
+  current_database,it means using the name of the connecting database.
+ 
+

For commands that accept the CURRENT_USER and SESSION_USER
keywords, the keywords are typically listed in the 'Synopsis'
section.  I think CURRENT_DATABASE should be no different.  For
example, the parameter type above could be
"database_specification," and the following definition could be
included at the bottom of the synopsis:

where database_specification can be:

object_name
  | CURRENT_DATABASE

Then, in the parameters section, the CURRENT_DATABASE keyword
would be defined:

CURRENT_DATABASE

Comment on the current database instead of an
explicitly identified role.

Also, it looks like only the COMMENT and SECURITY LABEL
documentation is being updated in this patch set.  However, this
keyword seems applicable to many other commands, too (e.g.
GRANT, ALTER DATABASE, ALTER ROLE, etc.).

+static ObjectAddress
+get_object_address_database(ObjectType objtype, DbSpec *object, bool 
missing_ok)
+{
+   char*dbname;
+   ObjectAddress   address;
+
+   dbname = get_dbspec_name(object);
+
+   address.classId = DatabaseRelationId;
+   address.objectId = get_database_oid(dbname, missing_ok);
+   address.objectSubId = 0;
+
+   return address;
+}

This helper function is only used once, and it seems simple
enough to build the ObjectAddress in the switch statement.
Also, instead of get_database_oid(), could we use
get_dbspec_oid()?

if (stmt->objtype == OBJECT_DATABASE)
{
-   char   *database = strVal((Value *) stmt->object);
-
-   if (!OidIsValid(get_database_oid(database, true)))
+   if (!OidIsValid(get_dbspec_oid((DbSpec*)stmt->object, true)))
{
+   char*dbname = NULL;
+
+   dbname = get_dbspec_name((DbSpec*)stmt->object);
+
ereport(WARNING,
(errcode(ERRCODE_UNDEFINED_DATABASE),
-errmsg("database \"%s\" does not 
exist", database)));
+errmsg("database \"%s\" does not 
exist", dbname)));
return address;
}
}

This section seems to assume that the DbSpec will be of type
DBSPEC_CSTRING in the error handling.  That should be safe for
now, as you cannot drop the current database, but I would
suggest adding assertions here to be sure.

+   dbname = get_dbspec_name((DbSpec*)stmt->dbspec);

As a general note, casts are typically styled as "(DbSpec *)
stmt" (with the spaces) in PostgreSQL.

+   case DBSPEC_CURRENT_DATABASE:
+   {
+   HeapTuple   tuple;
+   Form_pg_database dbForm;

Can we just declare "tuple" and "dbForm" at the beginning of
get_dbspec_name() so we don't need the extra set of braces?

+   if (fout->remoteVersion >= 10)
+   appendPQExpBuffer(dbQry, "COMMENT ON DATABASE 
CURRENT_DATABASE IS ");
+   else
+   appendPQExpBuffer(dbQry, "COMMENT ON DATABASE 
%s IS ", fmtId(datname));

This feature would probably only be added to v11, so the version
checks in the pgdump patch will need to be updated.

Nathan


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