Re: Fast COPY FROM based on batch insert
On 09/07/2022 00:09, Andrey Lepikhov wrote: On 8/7/2022 05:12, Ian Barwick wrote: ERROR: bind message supplies 0 parameters, but prepared statement "pgsql_fdw_prep_178" requires 6 CONTEXT: remote SQL command: INSERT INTO public.foo_part_1(t, v1, v2, v3, v4, v5) VALUES ($1, $2, $3, $4, $5, $6) COPY foo, line 88160 Thanks, I got it. MultiInsertBuffer are created on the first non-zero flush of tuples into the partition and isn't deleted from the buffers list until the end of COPY. And on a subsequent flush in the case of empty buffer we catch the error. Your fix is correct, but I want to propose slightly different change (see in attachment). LGTM. Regards Ian Barwick -- https://www.enterprisedb.com/
Re: Fast COPY FROM based on batch insert
On 07/07/2022 22:51, Andrey Lepikhov wrote:
> On 7/7/2022 06:14, Ian Barwick wrote:
>> 2022年3月24日(木) 15:44 Andrey V. Lepikhov :
>> >
>> > On 3/22/22 06:54, Etsuro Fujita wrote:
>> > > On Fri, Jun 4, 2021 at 5:26 PM Andrey Lepikhov
>> > > wrote:
>> > >> We still have slow 'COPY FROM' operation for foreign tables in current
>> > >> master.
>> > >> Now we have a foreign batch insert operation And I tried to rewrite the
>> > >> patch [1] with this machinery.
>> > >
>> > > The patch has been rewritten to something essentially different, but
>> > > no one reviewed it. (Tsunakawa-san gave some comments without looking
>> > > at it, though.) So the right status of the patch is “Needs review”,
>> > > rather than “Ready for Committer”? Anyway, here are a few review
>> > > comments from me:
>> > >
>> > > * I don’t think this assumption is correct:
>> > >
>> > > @@ -359,6 +386,12 @@ CopyMultiInsertBufferFlush(CopyMultiInsertInfo
*miinfo,
>> > > (resultRelInfo->ri_TrigDesc->trig_insert_after_row ||
>> > >resultRelInfo->ri_TrigDesc->trig_insert_new_table))
>> > > {
>> > > + /*
>> > > +* AFTER ROW triggers aren't allowed with the foreign bulk
insert
>> > > +* method.
>> > > +*/
>> > > + Assert(resultRelInfo->ri_RelationDesc->rd_rel->relkind !=
>> > > RELKIND_FOREIGN_TABLE);
>> > > +
>> > >
>> > > In postgres_fdw we disable foreign batch insert when the target table
>> > > has AFTER ROW triggers, but the core allows it even in that case. No?
>> > Agree
>> >
>> > > * To allow foreign multi insert, the patch made an invasive change to
>> > > the existing logic to determine whether to use multi insert for the
>> > > target relation, adding a new member ri_usesMultiInsert to the
>> > > ResultRelInfo struct, as well as introducing a new function
>> > > ExecMultiInsertAllowed(). But I’m not sure we really need such a
>> > > change. Isn’t it reasonable to *adjust* the existing logic to allow
>> > > foreign multi insert when possible?
>> > Of course, such approach would look much better, if we implemented it.
>> > I'll ponder how to do it.
>> >
>> > > I didn’t finish my review, but I’ll mark this as “Waiting on Author”.
>> > I rebased the patch onto current master. Now it works correctly. I'll
>> > mark it as "Waiting for review".
>>
>> I took a look at this patch as it would a useful optimization to have.
>>
>> It applies cleanly to current HEAD, but as-is, with a large data set, it
>> reproducibly fails like this (using postgres_fdw):
>>
>> postgres=# COPY foo FROM '/tmp/fast-copy-from/test.csv' WITH (format
csv);
>> ERROR: bind message supplies 0 parameters, but prepared statement
"pgsql_fdw_prep_19422" requires 6
>> CONTEXT: remote SQL command: INSERT INTO public.foo_part_1(t, v1, v2,
v3, v4, v5) VALUES ($1, $2, $3, $4, $5, $6)
>> COPY foo, line 17281589
>>
>> This occurs because not all multi-insert buffers being flushed actually
contain
>> tuples; the fix is simply not to call ExecForeignBatchInsert() if that's the
case,
>> e.g:
>>
>>
>> /* Flush into foreign table or partition */
>> do {
>> int size = (resultRelInfo->ri_BatchSize < nused - sent) ?
>> resultRelInfo->ri_BatchSize : (nused - sent);
>>
>> if (size)
>> {
>> int inserted = size;
>>
>> resultRelInfo->ri_FdwRoutine->ExecForeignBatchInsert(estate,
>> resultRelInfo,
>> [sent],
>> NULL,
>> );
>> sent += size;
>> }
>> } while (sent < nused);
>>
>>
>> There might a case for arguing that the respective FDW should check that it
has
>> actually received some tuples to insert, but IMHO it's much preferable to
catch
>> this as early as possible and avoid a superfluous call.
>>
>> FWIW, with the above fix in place, with a simple local test the patch
produces a
>> consistent speed-up of about 8 times compared to the existing functionality.
>
> Thank you for the
Re: Fast COPY FROM based on batch insert
2022年3月24日(木) 15:44 Andrey V. Lepikhov :
>
> On 3/22/22 06:54, Etsuro Fujita wrote:
> > On Fri, Jun 4, 2021 at 5:26 PM Andrey Lepikhov
> > wrote:
> >> We still have slow 'COPY FROM' operation for foreign tables in current
> >> master.
> >> Now we have a foreign batch insert operation And I tried to rewrite the
> >> patch [1] with this machinery.
> >
> > The patch has been rewritten to something essentially different, but
> > no one reviewed it. (Tsunakawa-san gave some comments without looking
> > at it, though.) So the right status of the patch is “Needs review”,
> > rather than “Ready for Committer”? Anyway, here are a few review
> > comments from me:
> >
> > * I don’t think this assumption is correct:
> >
> > @@ -359,6 +386,12 @@ CopyMultiInsertBufferFlush(CopyMultiInsertInfo *miinfo,
> > (resultRelInfo->ri_TrigDesc->trig_insert_after_row ||
> >resultRelInfo->ri_TrigDesc->trig_insert_new_table))
> > {
> > + /*
> > +* AFTER ROW triggers aren't allowed with the foreign bulk
insert
> > +* method.
> > +*/
> > + Assert(resultRelInfo->ri_RelationDesc->rd_rel->relkind !=
> > RELKIND_FOREIGN_TABLE);
> > +
> >
> > In postgres_fdw we disable foreign batch insert when the target table
> > has AFTER ROW triggers, but the core allows it even in that case. No?
> Agree
>
> > * To allow foreign multi insert, the patch made an invasive change to
> > the existing logic to determine whether to use multi insert for the
> > target relation, adding a new member ri_usesMultiInsert to the
> > ResultRelInfo struct, as well as introducing a new function
> > ExecMultiInsertAllowed(). But I’m not sure we really need such a
> > change. Isn’t it reasonable to *adjust* the existing logic to allow
> > foreign multi insert when possible?
> Of course, such approach would look much better, if we implemented it.
> I'll ponder how to do it.
>
> > I didn’t finish my review, but I’ll mark this as “Waiting on Author”.
> I rebased the patch onto current master. Now it works correctly. I'll
> mark it as "Waiting for review".
I took a look at this patch as it would a useful optimization to have.
It applies cleanly to current HEAD, but as-is, with a large data set, it
reproducibly fails like this (using postgres_fdw):
postgres=# COPY foo FROM '/tmp/fast-copy-from/test.csv' WITH (format csv);
ERROR: bind message supplies 0 parameters, but prepared statement
"pgsql_fdw_prep_19422" requires 6
CONTEXT: remote SQL command: INSERT INTO public.foo_part_1(t, v1, v2, v3,
v4, v5) VALUES ($1, $2, $3, $4, $5, $6)
COPY foo, line 17281589
This occurs because not all multi-insert buffers being flushed actually contain
tuples; the fix is simply not to call ExecForeignBatchInsert() if that's the
case,
e.g:
/* Flush into foreign table or partition */
do {
int size = (resultRelInfo->ri_BatchSize < nused - sent) ?
resultRelInfo->ri_BatchSize : (nused - sent);
if (size)
{
int inserted = size;
resultRelInfo->ri_FdwRoutine->ExecForeignBatchInsert(estate,
resultRelInfo,
[sent],
NULL,
);
sent += size;
}
} while (sent < nused);
There might a case for arguing that the respective FDW should check that it has
actually received some tuples to insert, but IMHO it's much preferable to catch
this as early as possible and avoid a superfluous call.
FWIW, with the above fix in place, with a simple local test the patch produces a
consistent speed-up of about 8 times compared to the existing functionality.
Regards
Ian Barwick
--
EnterpriseDB - https://www.enterprisedb.com
Re: Corner-case bug in pg_rewind
On 02/12/2020 20:13, Heikki Linnakangas wrote: On 01/12/2020 16:52, Pavel Borisov wrote: Status update for a commitfest entry. The patch is Waiting on Author for some time. As this is a bug fix, I am moving it to the next CF. Ian, are you planning to continue working on it? As a reviewer, I consider the patch useful and good overall. The comments I left were purely cosmetic. It's a pity to me that this bugfix delayed for such a small reason and outdated, therefore. It would be nice to complete this fix on the next CF. Yeah, we really should fix this.. On 16/11/2020 04:49, Ian Lawrence Barwick wrote: Also, I think Heikki's notion could be fulfilled. I spent a bit of time looking at that suggestion but couldn't actually verify it was an issue which needed fixing. > Attached are two patches. The first patch is your original patch, unmodified (except for a cosmetic rename of the test file). The second patch builds on that, demonstrating and fixing the issue I mentioned. It took me a while to create a repro for it, it's easily masked by incidental full-page writes or because rows created by XIDs that are not marked as committed on the other timeline are invisible, but succeeded at last. Aha, many thanks. I wasn't entirely sure what I was looking for there and recently haven't had the time or energy to dig any further. Regards Ian Barwick -- Ian Barwick https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: Improving connection scalability: GetSnapshotData()
On 2020/10/02 3:26, Andres Freund wrote: Hi Ian, Andrew, All, On 2020-09-30 15:43:17 -0700, Andres Freund wrote: Attached is an updated version of the test (better utility function, stricter regexes, bailing out instead of failing just the current when psql times out). I'm leaving it in this test for now, but it's fairly easy to use this way, in my opinion, so it may be worth moving to PostgresNode at some point. I pushed this yesterday. Ian, thanks again for finding this and helping with fixing & testing. Thanks! Apologies for not getting back to your earlier responses, have been distracted by Various Other Things. The tests I run which originally triggered the issue now run just fine. Regards Ian Barwick -- Ian Barwick https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Corner-case bug in pg_rewind
Hi
Take the following cluster with:
- node1 (initial primary)
- node2 (standby)
- node3 (standby)
Following activity takes place (greatly simplified from a real-world situation):
1. node1 is shut down.
2. node3 is promoted
3. node2 is attached to node3.
4. node1 is attached to node3
5. node1 is then promoted (creating a split brain situation with
node1 and node3 as primaries)
6. node2 and node3 are shut down (in that order).
7. pg_rewind is executed to reset node2 so it can reattach
to node1 as a standby. pg_rewind claims:
pg_rewind: servers diverged at WAL location X/XXX on timeline 2
pg_rewind: no rewind required
8. based off that assurance, node2 is restarted with replication configuration
pointing to node1 - but it is unable to attach, with node2's log reporting
something like:
new timeline 3 forked off current database system timeline 2
before current recovery point X/XXX
The cause is that pg_rewind is assuming that if the node's last
checkpoint matches the
divergence point, no rewind is needed:
if (chkptendrec == divergerec)
rewind_needed = false;
but in this case there *are* records beyond the last checkpoint, which can be
inferred from "minRecoveryPoint" - but this is not checked.
Attached patch addresses this. It includes a test, which doesn't make use of
the RewindTest module, as that hard-codes a primary and a standby, while here
three nodes are needed (I can't come up with a situation where this can be
reproduced with only two nodes). The test sets "wal_keep_size" so would need
modification for Pg12 and earlier.
Regards
Ian Barwick
--
Ian Barwick https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
commit ec0465014825628ec9b868703444214ac4738c53
Author: Ian Barwick
Date: Fri Sep 11 10:13:17 2020 +0900
pg_rewind: catch corner-case situation
It's possible that a standby, after diverging from the source node,
is shut down without a shutdown checkpoint record, and the divergence
point matches a shutdown checkpoint from a previous shutdown. In this
case the presence of WAL records beyond the shutdown checkpoint (asi
indicated by minRecoveryPoint) needs to be detected in order to
determine whether a rewind is needed.
diff --git a/src/bin/pg_rewind/pg_rewind.c b/src/bin/pg_rewind/pg_rewind.c
index 23fc749e44..393c8ebbcd 100644
--- a/src/bin/pg_rewind/pg_rewind.c
+++ b/src/bin/pg_rewind/pg_rewind.c
@@ -342,13 +342,20 @@ main(int argc, char **argv)
targetNentries - 1,
restore_command);
+ /*
+ * If the minimum recovery ending location is beyond the end of
+ * the last checkpoint, that means there are WAL records beyond
+ * the divergence point and a rewind is needed.
+ */
+ if (ControlFile_target.minRecoveryPoint > chkptendrec)
+rewind_needed = true;
/*
* If the histories diverged exactly at the end of the shutdown
* checkpoint record on the target, there are no WAL records in
* the target that don't belong in the source's history, and no
* rewind is needed.
*/
- if (chkptendrec == divergerec)
+ else if (chkptendrec == divergerec)
rewind_needed = false;
else
rewind_needed = true;
diff --git a/src/bin/pg_rewind/t/007_min_recovery_point.pl b/src/bin/pg_rewind/t/007_min_recovery_point.pl
new file mode 100644
index 00..ac842fd0ed
--- /dev/null
+++ b/src/bin/pg_rewind/t/007_min_recovery_point.pl
@@ -0,0 +1,118 @@
+#
+# Test situation where a target data directory contains
+# WAL records beyond both the last checkpoint and the divergence
+# point.
+#
+# This test does not make use of RewindTest as it requires three
+# nodes.
+
+use strict;
+use warnings;
+use PostgresNode;
+use TestLib;
+use Test::More tests => 2;
+
+my $node_1 = get_new_node('node_1');
+$node_1->init(allows_streaming => 1);
+$node_1->append_conf(
+ 'postgresql.conf', qq(
+wal_keep_size=16
+));
+
+$node_1->start;
+
+# Add an arbitrary table
+$node_1->safe_psql('postgres',
+ 'CREATE TABLE public.foo (id INT)');
+
+# Take backup
+my $backup_name = 'my_backup';
+$node_1->backup($backup_name);
+
+# Create streaming standby from backup
+my $node_2 = get_new_node('node_2');
+$node_2->init_from_backup($node_1, $backup_name,
+ has_streaming => 1);
+
+$node_2->append_conf(
+ 'postgresql.conf', qq(
+wal_keep_size=16
+));
+
+$node_2->start;
+
+# Create streaming standby from backup
+my $node_3 = get_new_node('node_3');
+$node_3->init_from_backup($node_1, $backup_name,
+ has_streaming => 1);
+
+$node_3->append_conf(
+ 'postgresql.conf', qq(
+wal_keep_size=16
+));
+
+$node_3->start;
+
+# Stop node_1
+
+$node_1->stop('fast');
+
+# Promote node_3
+$node_3->promote;
+
+# node_1 rejoins node_3
+
+my $node_3_connstr = $node_3->connstr;
+
+$node_1->append_conf(
+ 'postgresql.conf', qq(
+primary_conninfo='$node_3_conns
Re: Improving connection scalability: GetSnapshotData()
On 2020/09/08 13:23, Ian Barwick wrote: On 2020/09/08 13:11, Andres Freund wrote: Hi, On 2020-09-08 13:03:01 +0900, Ian Barwick wrote: (...) I wonder if it's possible to increment "xactCompletionCount" during replay along these lines: *** a/src/backend/access/transam/xact.c --- b/src/backend/access/transam/xact.c *** xact_redo_commit(xl_xact_parsed_commit * *** 5915,5920 --- 5915,5924 */ if (XactCompletionApplyFeedback(parsed->xinfo)) XLogRequestWalReceiverReply(); + + LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE); + ShmemVariableCache->xactCompletionCount++; + LWLockRelease(ProcArrayLock); } which seems to work (though quite possibly I've overlooked something I don't know that I don't know about and it will all break horribly somewhere, etc. etc.). We'd also need the same in a few more places. Probably worth looking at the list where we increment it on the primary (particularly we need to also increment it for aborts, and 2pc commit/aborts). Yup. At first I was very confused as to why none of the existing tests have found this significant issue. But after thinking about it for a minute that's because they all use psql, and largely separate psql invocations for each query :(. Which means that there's no cached snapshot around... Do you want to try to write a patch? Sure, I'll give it a go as I have some time right now. Attached, though bear in mind I'm not very familiar with parts of this, particularly 2PC stuff, so consider it educated guesswork. Regards Ian Barwick -- Ian Barwick https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services commit 544e2b1661413fe08e3083f03063c12c0d7cf3aa Author: Ian Barwick Date: Tue Sep 8 12:24:14 2020 +0900 Fix snapshot caching on standbys Addresses issue introduced in 623a9ba. diff --git a/src/backend/access/transam/multixact.c b/src/backend/access/transam/multixact.c index b8bedca04a..227d03bbce 100644 --- a/src/backend/access/transam/multixact.c +++ b/src/backend/access/transam/multixact.c @@ -3320,6 +3320,14 @@ multixact_redo(XLogReaderState *record) } else elog(PANIC, "multixact_redo: unknown op code %u", info); + + /* + * Advance xactCompletionCount so rebuilds of snapshot contents + * can be triggered. + */ + LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE); + ShmemVariableCache->xactCompletionCount++; + LWLockRelease(ProcArrayLock); } Datum diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c index af6afcebb1..04ca858918 100644 --- a/src/backend/access/transam/xact.c +++ b/src/backend/access/transam/xact.c @@ -5915,6 +5915,14 @@ xact_redo_commit(xl_xact_parsed_commit *parsed, */ if (XactCompletionApplyFeedback(parsed->xinfo)) XLogRequestWalReceiverReply(); + + /* + * Advance xactCompletionCount so rebuilds of snapshot contents + * can be triggered. + */ + LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE); + ShmemVariableCache->xactCompletionCount++; + LWLockRelease(ProcArrayLock); } /* @@ -5978,6 +5986,14 @@ xact_redo_abort(xl_xact_parsed_abort *parsed, TransactionId xid) /* Make sure files supposed to be dropped are dropped */ DropRelationFiles(parsed->xnodes, parsed->nrels, true); + + /* + * Advance xactCompletionCount so rebuilds of snapshot contents + * can be triggered. + */ + LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE); + ShmemVariableCache->xactCompletionCount++; + LWLockRelease(ProcArrayLock); } void
Re: Improving connection scalability: GetSnapshotData()
On 2020/09/09 2:53, Andres Freund wrote:
Hi,
On 2020-09-08 16:44:17 +1200, Thomas Munro wrote:
On Tue, Sep 8, 2020 at 4:11 PM Andres Freund wrote:
At first I was very confused as to why none of the existing tests have
found this significant issue. But after thinking about it for a minute
that's because they all use psql, and largely separate psql invocations
for each query :(. Which means that there's no cached snapshot around...
I prototyped a TAP test patch that could maybe do the sort of thing
you need, in patch 0006 over at [1]. Later versions of that patch set
dropped it, because I figured out how to use the isolation tester
instead, but I guess you can't do that for a standby test (at least
not until someone teaches the isolation tester to support multi-node
schedules, something that would be extremely useful...).
Unfortunately proper multi-node isolationtester test basically is
equivalent to building a global lock graph. I think, at least? Including
a need to be able to correlate connections with their locks between the
nodes.
But for something like the bug at hand it'd probably sufficient to just
"hack" something with dblink. In session 1) insert a row on the primary
using dblink, return the LSN, wait for the LSN to have replicated and
finally in session 2) check for row visibility.
The attached seems to do the trick.
Regards
Ian Barwick
--
Ian Barwick https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
commit b31d587d71f75115b02dd1bf6230a56722c67832
Author: Ian Barwick
Date: Wed Sep 9 14:37:40 2020 +0900
test for standby row visibility
diff --git a/src/test/recovery/Makefile b/src/test/recovery/Makefile
index fa8e031526..2d9a9701fc 100644
--- a/src/test/recovery/Makefile
+++ b/src/test/recovery/Makefile
@@ -9,7 +9,7 @@
#
#-
-EXTRA_INSTALL=contrib/test_decoding
+EXTRA_INSTALL=contrib/test_decoding contrib/dblink
subdir = src/test/recovery
top_builddir = ../../..
diff --git a/src/test/recovery/t/021_row_visibility.pl b/src/test/recovery/t/021_row_visibility.pl
new file mode 100644
index 00..5f591d131e
--- /dev/null
+++ b/src/test/recovery/t/021_row_visibility.pl
@@ -0,0 +1,84 @@
+# Checks that a standby session can see all expected rows
+use strict;
+use warnings;
+
+use PostgresNode;
+use TestLib;
+use Test::More tests => 1;
+
+# Initialize primary node
+my $node_primary = get_new_node('primary');
+$node_primary->init(allows_streaming => 1);
+$node_primary->start;
+
+$node_primary->safe_psql('postgres',
+ 'CREATE EXTENSION dblink');
+
+
+# Add an arbitrary table
+$node_primary->safe_psql('postgres',
+ 'CREATE TABLE public.foo (id INT)');
+
+# Take backup
+my $backup_name = 'my_backup';
+$node_primary->backup($backup_name);
+
+# Create streaming standby from backup
+my $node_standby = get_new_node('standby');
+$node_standby->init_from_backup($node_primary, $backup_name,
+ has_streaming => 1);
+$node_standby->start;
+
+sleep(5);
+# Check row visibility in an existing standby session
+
+my ($res, $stdout, $stderr) = $node_standby->psql(
+'postgres',
+sprintf(
+<<'EO_SQL',
+DO $$
+ DECLARE
+primary_lsn pg_lsn;
+insert_xmin xid;
+standby_rec RECORD;
+ BEGIN
+SELECT INTO primary_lsn, insert_xmin
+ t1.primary_lsn, t1.xmin
+ FROM dblink(
+ 'host=%s port=%i dbname=postgres',
+ 'INSERT INTO public.foo VALUES (1) RETURNING pg_catalog.pg_current_wal_lsn(), xmin'
+ ) AS t1(primary_lsn pg_lsn, xmin xid);
+
+LOOP
+ EXIT WHEN pg_catalog.pg_last_wal_replay_lsn() > primary_lsn;
+END LOOP;
+
+SELECT INTO standby_rec
+ id
+ FROM public.foo
+ WHERE id = 1 AND xmin = insert_xmin;
+
+IF FOUND
+ THEN
+RAISE NOTICE 'row found';
+ ELSE
+RAISE NOTICE 'row not found';
+END IF;
+
+ END;
+$$;
+EO_SQL
+$node_primary->host,
+$node_primary->port,
+),
+);
+
+
+like (
+$stderr,
+qr/row found/,
+'check that inserted row is visible on the standby',
+);
+
+$node_primary->stop;
+$node_standby->stop;
Re: file_fdw vs relative paths
Hi
On 2020/09/07 2:31, Magnus Hagander wrote:
On Mon, Aug 31, 2020 at 5:03 PM Bruce Momjian mailto:br...@momjian.us>> wrote:
On Mon, Aug 31, 2020 at 01:16:05PM +0200, Magnus Hagander wrote:
> Bruce, I've applied and backpatched your docs patch for this.
>
> Gah, and of course right after doing that, I remembered I wanted to get a
> second change in :) To solve the "who's this Josh" question, I suggest
we also
> change the example to point to the data/log directory which is likely to
exist
> in a lot more of the cases. I keep getting people who ask "who is josh"
based
> on the /home/josh path. Not that it's that important, but...
Thanks, and agreed.
Thanks, applied. I backpacked to 13 but didn't bother with the rest as it's not
technically *wrong* before..
It's missing the leading single quote from the filename parameter:
diff --git a/doc/src/sgml/file-fdw.sgml b/doc/src/sgml/file-fdw.sgml
(...)
-OPTIONS ( filename '/home/josh/data/log/pglog.csv', format 'csv' );
+OPTIONS ( filename log/pglog.csv', format 'csv' );
(...)
Regards
Ian Barwick
--
Ian Barwick https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
Re: Improving connection scalability: GetSnapshotData()
On 2020/09/08 13:11, Andres Freund wrote: Hi, On 2020-09-08 13:03:01 +0900, Ian Barwick wrote: (...) I wonder if it's possible to increment "xactCompletionCount" during replay along these lines: *** a/src/backend/access/transam/xact.c --- b/src/backend/access/transam/xact.c *** xact_redo_commit(xl_xact_parsed_commit * *** 5915,5920 --- 5915,5924 */ if (XactCompletionApplyFeedback(parsed->xinfo)) XLogRequestWalReceiverReply(); + + LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE); + ShmemVariableCache->xactCompletionCount++; + LWLockRelease(ProcArrayLock); } which seems to work (though quite possibly I've overlooked something I don't know that I don't know about and it will all break horribly somewhere, etc. etc.). We'd also need the same in a few more places. Probably worth looking at the list where we increment it on the primary (particularly we need to also increment it for aborts, and 2pc commit/aborts). Yup. At first I was very confused as to why none of the existing tests have found this significant issue. But after thinking about it for a minute that's because they all use psql, and largely separate psql invocations for each query :(. Which means that there's no cached snapshot around... Do you want to try to write a patch? Sure, I'll give it a go as I have some time right now. Regards Ian Barwick -- Ian Barwick https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: Improving connection scalability: GetSnapshotData()
On 2020/09/03 17:18, Michael Paquier wrote: On Sun, Aug 16, 2020 at 02:26:57PM -0700, Andres Freund wrote: So we get some builfarm results while thinking about this. Andres, there is an entry in the CF for this thread: https://commitfest.postgresql.org/29/2500/ A lot of work has been committed with 623a9ba, 73487a6, 5788e25, etc. I haven't seen it mentioned here, so apologies if I've overlooked something, but as of 623a9ba queries on standbys seem somewhat broken. Specifically, I maintain some code which does something like this: - connects to a standby - checks a particular row does not exist on the standby - connects to the primary - writes a row in the primary - polls the standby (using the same connection as above) to verify the row arrives on the standby As of recent HEAD it never sees the row arrive on the standby, even though it is verifiably there. I've traced this back to 623a9ba, which relies on "xactCompletionCount" being incremented to determine whether the snapshot can be reused, but that never happens on a standby, meaning this test in GetSnapshotDataReuse(): if (curXactCompletionCount != snapshot->snapXactCompletionCount) return false; will never return false, and the snapshot's xmin/xmax never get advanced. Which means the session on the standby is not able to see rows on the standby added after the session was started. It's simple enough to prevent that being an issue by just never calling GetSnapshotDataReuse() if the snapshot was taken during recovery (though obviously that means any performance benefits won't be available on standbys). I wonder if it's possible to increment "xactCompletionCount" during replay along these lines: *** a/src/backend/access/transam/xact.c --- b/src/backend/access/transam/xact.c *** xact_redo_commit(xl_xact_parsed_commit * *** 5915,5920 --- 5915,5924 */ if (XactCompletionApplyFeedback(parsed->xinfo)) XLogRequestWalReceiverReply(); + + LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE); + ShmemVariableCache->xactCompletionCount++; + LWLockRelease(ProcArrayLock); } which seems to work (though quite possibly I've overlooked something I don't know that I don't know about and it will all break horribly somewhere, etc. etc.). Regards Ian Barwick -- Ian Barwick https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
[doc] minor wording improvement in a couple of places
Hi On these pages: https://www.postgresql.org/docs/current/fdw-callbacks.html https://www.postgresql.org/docs/current/tablesample-method.html we have the phrase: "..., which see for additional details." which strikes me as a bit odd. Suggested phrasing: "...; see this file for additional details." Regards Ian Barwick -- Ian Barwick https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services diff --git a/doc/src/sgml/fdwhandler.sgml b/doc/src/sgml/fdwhandler.sgml new file mode 100644 index 72fa127..1433552 *** a/doc/src/sgml/fdwhandler.sgml --- b/doc/src/sgml/fdwhandler.sgml *** *** 91,97 The FdwRoutine struct type is declared in ! src/include/foreign/fdwapi.h, which see for additional details. --- 91,97 The FdwRoutine struct type is declared in ! src/include/foreign/fdwapi.h; see this file for additional details. diff --git a/doc/src/sgml/tablesample-method.sgml b/doc/src/sgml/tablesample-method.sgml new file mode 100644 index 1c9f1bf..bdc60a0 *** a/doc/src/sgml/tablesample-method.sgml --- b/doc/src/sgml/tablesample-method.sgml *** method_name(internal) RETURNS tsm_handle *** 87,93 The TsmRoutine struct type is declared ! in src/include/access/tsmapi.h, which see for additional details. --- 87,93 The TsmRoutine struct type is declared ! in src/include/access/tsmapi.h; see this file for additional details.
Re: Manager for commit fest 2020-09
On 2020/09/01 10:43, Michael Paquier wrote: On Mon, Aug 31, 2020 at 04:37:12PM +0900, Michael Paquier wrote: We are going to be in September in a couple of hours, meaning that the second commit fest for Postgres 14 will begin soon. Do we have any volunteers to take the role of CFM this time? As of the moment this message is written, 10 hours remain until we are the 1st of September AoE, where I'll switch the commit fest as officially in progress. It will not be possible to register new patches to 2020-09 after that. 2020-11 is also now showing as "in progress", is that correct? Regards Ian Barwick -- Ian Barwick https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: pg13 docs: minor fix for "System views" list
On 2020/05/25 16:03, Fujii Masao wrote: On 2020/05/25 15:24, Michael Paquier wrote: On Mon, May 25, 2020 at 03:12:57PM +0900, Fujii Masao wrote: Thanks! LGTM. Will commit this. Oops :) No problem :) Thanks for the commit! Thanks both! Regards Ian Barwick -- Ian Barwick https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
pg13 docs: minor fix for "System views" list
Hi In this list: https://www.postgresql.org/docs/devel/views-overview.html "pg_shmem_allocations" is not quite in alphabetical order and needs to be swapped with the preceding entry, per attached patch. Regards Ian Barwick -- Ian Barwick https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services commit 76dccbb12463f2c5195cd09d62cacb69dc630fb6 Author: Ian Barwick Date: Mon May 25 10:57:05 2020 +0900 doc: fix alphabetical order of "System views" overview "pg_shmem_allocations" was in the wrong place. diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml index 9851ef2713..70fda9408b 100644 --- a/doc/src/sgml/catalogs.sgml +++ b/doc/src/sgml/catalogs.sgml @@ -9309,13 +9309,13 @@ SCRAM-SHA-256$iteration count: - pg_stats - planner statistics + pg_shmem_allocations + shared memory allocations - pg_shmem_allocations - shared memory allocations + pg_stats + planner statistics
Re: Remove non-fast promotion Re: Should we remove a fallback promotion? take 2
On 2020/04/22 6:53, Alvaro Herrera wrote: On 2020-Apr-21, Jehan-Guillaume de Rorthais wrote: On Tue, 21 Apr 2020 15:36:22 +0900 Michael Paquier wrote: Also in that case, non-fast promotion is triggered. Since my patch tries to remove non-fast promotion, it's intentional to prevent them from doing that. But you think that we should not drop that because there are still some users for that? It would be good to ask around to folks maintaining HA solutions about that change at least, as there could be a point in still letting promotion to happen in this case, but switch silently to the fast path. FWIW, PAF relies on pg_ctl promote. No need for non-fast promotion. AFAICT repmgr uses 'pg_ctl promote', and has since version 3.0 (released in mid 2015). It was only 3.3.2 (mid 2017) that supported Postgres 10, so it seems fairly safe to assume that the removal won't be a problem. Correct, repmgr uses "pg_ctl promote" or pg_promote() (if available), and won't be affected by this change. Regards Ian Barwick -- Ian Barwick https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: Standby got fatal after the crash recovery
On 2020/03/17 12:53, Thunder wrote: Sorry. We are using pg11, and cloned from tag REL_11_BETA2. In that case you should upgrade to the current version in the PostgreSQL 11 series (at the time of writing 11.7). Regards Ian Barwick -- Ian Barwick https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: Add %x to PROMPT1 and PROMPT2
On 2020/02/03 23:40, Tom Lane wrote: Daniel Gustafsson writes: On 3 Feb 2020, at 08:08, Michael Paquier wrote: FWIW, I am not really in favor of changing a default old enough that it could vote (a45195a). That by itself doesn't seem a good reason to not change things. My concern would be that users who have never ever considered that the prompt can be changed, all of sudden wonder why the prompt is showing characters it normally isn't, thus causing confusion. That being said, I agree that this is a better default long-term. I've got the same misgivings as Michael. In a green field this'd likely be a good idea, but after so many years I'm afraid it will make fewer people happy than unhappy. Now on the other hand, we did change the server's default log_line_prefix not so long ago (7d323to se5ba4), and the feared storm of complaints was pretty much undetectable. So maybe this'd go down the same way. Worth noting also is that this shouldn't be able to break any applications, since the prompt is an interactive-only behavior. The last change I recall affecting default psql behaviour was the addition of COMP_KEYWORD_CASE in 9.2 (db84ba65), which personally I (and no doubt others) found annoying, but the world still turns. +1 one for this change, it's something I also add to every .psqlrc I setup. Moreover, some of my work involves logging in at short notice to other people's systems where I don't have control over the .psqlrc setup - while I can of course set the prompt manually it's an extra step, and it would be really nice to see the transaction status by default when working on critical systems in a time-critical situation. (Obviously it'll take a few years for this change to filter into production...). Regards Ian Barwick -- Ian Barwick https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: Prevent pg_basebackup running as root
On 2020/02/01 18:34, Michael Paquier wrote: On Thu, Jan 30, 2020 at 04:00:40PM +0900, Michael Paquier wrote: Anyway, your patch looks like a good idea to me, so let's see if others have opinions or objections about it. Seeing nothing, committed v2. Thanks! Regards Ian Barwick -- Ian Barwick https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: Prevent pg_basebackup running as root
2020年1月30日(木) 14:57 Michael Paquier :
>
> On Thu, Jan 30, 2020 at 02:29:06PM +0900, Ian Barwick wrote:
> > I can't think of any practical reason why pg_basebackup would ever need to
> > be run as root; we disallow that for initdb, pg_ctl and pg_upgrade, so it
> > seems reasonable to do the same for pg_basebackup. Trivial patch attached,
> > which as with the other cases will allow only the --help/--version options
> > to be executed as root, otherwise nothing else.
>
> My take on the matter is that we should prevent anything creating or
> modifying the data directory to run as root if we finish with
> permissions incompatible with what a postmaster expects. So +1.
>
> > The patch doesn't update the pg_basebackup documentation page; we don't
> > mention it in the pg_ctl and pg_upgrade pages either and it doesn't seem
> > particularly important to mention it explicitly.
>
> We don't mention that in the docs of pg_rewind either. Note also that
> before 5d5aedd pg_rewind printed an error without exiting :)
Ouch.
> > + /*
> > + * Disallow running as root, as PostgreSQL will be unable to start
> > + * with root-owned files.
> > + */
>
> Here is a suggestion:
> /*
> * Don't allow pg_basebackup to be run as root, to avoid creating
> * files in the data directory with ownership rights incompatible
> * with the postmaster. We need only check for root -- any other user
> * won't have sufficient permissions to modify files in the data
> * directory.
> */
I think we can skip the second sentence altogether. It'd be theoretically
easy enough to up with some combination of group permissions,
sticky bits, umask, ACL settings etc/ which would allow one user to
modify the files owned by another user,
> > + #ifndef WIN32
>
> Indentation here.
Whoops, that's what comes from typing on the train ;)
> > + if (geteuid() == 0) /* 0 is root's uid */
> > + {
> > + pg_log_error("cannot be run as root");
> > + fprintf(stderr,
> > + _("Please log in (using, e.g., \"su\") as the
> > (unprivileged) user that will\n"
> > + "own the server process.\n"));
> > + exit(1);
> > + }
> > +#endif
>
> I would recommend to map with the existing message of pg_rewind for
> consistency:
> pg_log_error("cannot be executed by \"root\"");
> fprintf(stderr, _("You must run %s as the PostgreSQL superuser.\n"),
> progname);
Hmm, I was using the existing message from initdb and pg_ctl for consistency:
src/bin/initdb/initdb.c:
if (geteuid() == 0)/* 0 is root's uid */
{
pg_log_error("cannot be run as root");
fprintf(stderr,
_("Please log in (using, e.g., \"su\") as the
(unprivileged) user that will\n"
"own the server process.\n"));
exit(1);
}
src/bin/pg_ctl/pg_ctl.c:
if (geteuid() == 0)
{
write_stderr(_("%s: cannot be run as root\n"
"Please log in (using, e.g., \"su\") as the "
"(unprivileged) user that will\n"
"own the server process.\n"),
progname);
exit(1);
}
src/bin/pg_upgrade/option.c:
if (os_user_effective_id == 0)
pg_fatal("%s: cannot be run as root\n", os_info.progname);
I wonder if it would be worth settling on a common message and way of emitting
it, each utility does it slightly differently.
> A backpatch could be surprising for some users as that's a behavior
> change, so I would recommend not to do a backpatch.
Agreed.
Regards
Ian Barwick
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index 238b671f7a..dfa816cbae 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -2077,6 +2077,22 @@ main(int argc, char **argv)
}
}
+ /*
+ * Don't allow pg_basebackup to be run as root, to avoid creating
+ * files in the data directory with ownership rights incompatible
+ * with the postmaster.
+ */
+#ifndef WIN32
+ if (geteuid() == 0) /* 0 is root's uid */
+ {
+ pg_log_error("cannot be run as root");
+ fprintf(stderr,
+_("Please log in (using, e.g., \"su\") as the (unprivileged) user that will\n"
+ "own the server process.\n"));
+ exit(1);
+ }
+#endif
+
atexit(cleanup_directories_atexit);
while ((c = getopt_long(argc, argv, "CD:F:r:RS:T:X:l:nNzZ:d:c:h:p:U:s:wWkvP",
Prevent pg_basebackup running as root
Hi
We encountered an unfortunate case of $SUBJECT the other day where it
would have been preferable to catch the error before rather than after
pg_basebackup ran.
I can't think of any practical reason why pg_basebackup would ever need to
be run as root; we disallow that for initdb, pg_ctl and pg_upgrade, so it
seems reasonable to do the same for pg_basebackup. Trivial patch attached,
which as with the other cases will allow only the --help/--version options
to be executed as root, otherwise nothing else.
The patch doesn't update the pg_basebackup documentation page; we don't
mention it in the pg_ctl and pg_upgrade pages either and it doesn't seem
particularly important to mention it explicitly.
I'll add this to the March CF.
Regards
Ian Barwick
--
Ian Barwick https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index 238b671f7a..f25a137114 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -2077,6 +2077,21 @@ main(int argc, char **argv)
}
}
+ /*
+ * Disallow running as root, as PostgreSQL will be unable to start
+ * with root-owned files.
+ */
+ #ifndef WIN32
+ if (geteuid() == 0) /* 0 is root's uid */
+ {
+ pg_log_error("cannot be run as root");
+ fprintf(stderr,
+_("Please log in (using, e.g., \"su\") as the (unprivileged) user that will\n"
+ "own the server process.\n"));
+ exit(1);
+ }
+#endif
+
atexit(cleanup_directories_atexit);
while ((c = getopt_long(argc, argv, "CD:F:r:RS:T:X:l:nNzZ:d:c:h:p:U:s:wWkvP",
Re: doc: update PL/pgSQL sample loop function
On 2019/09/11 14:44, Amit Kapila wrote: On Sun, Sep 1, 2019 at 9:09 AM Amit Kapila wrote: The current example shows the usage of looping in plpgsql, so as such there is no correctness issue, but OTOH there is no harm in updating the example as proposed by Ian Barwick. Does anyone else see any problem with this idea? If we agree to proceed with this update, it might be better to backpatch it for the sake of consistency though I am not sure about that. While checking the patch in back-branches, I noticed that it doesn't get applied to 9.4 due to the way the example forms the string. I have done the required changes for 9.4 as well and attached is the result. Aha, I had it in my head that 9.4 was being deprecated soon and didn't check that far back, but turns out it's around until Feb. 2020. Ian, if possible, can you once check the patch for 9.4? Looks good, thanks for catching that! Regards Ian Barwick -- Ian Barwick https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: [PATCH] psql: add tab completion for \df slash command suffixes
On 2019/09/04 15:14, Kuntal Ghosh wrote:
Hello Ian,
On Fri, Aug 30, 2019 at 10:31 AM Ian Barwick
wrote:
I just noticed "\df[TAB]" fails to offer any tab-completion for
the possible suffixes ("\dfa", "\dfn", "\dfp", "\dft" and "\dfw").
Trivial patch attached, which applies back to Pg96, and separate
patches for Pg95 and Pg94.
I'll add this to the next commitfest.
I've reviewed the patch. It works as expected in master. But, the
syntax "\dfp" doesn't exist beyond Pg11. So, you've to remove the same
from the patches for Pg95 and Pg94. I guess the same patch should be
applied on Pg10 and Pg96.
Ah, good catch, I will update and resubmit.
Thanks!
Ian Barwick
--
Ian Barwick https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
[PATCH] psql: add tab completion for \df slash command suffixes
Hi
I just noticed "\df[TAB]" fails to offer any tab-completion for
the possible suffixes ("\dfa", "\dfn", "\dfp", "\dft" and "\dfw").
Trivial patch attached, which applies back to Pg96, and separate
patches for Pg95 and Pg94.
I'll add this to the next commitfest.
Regards
Ian Barwick
--
Ian Barwick https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
commit 41a4ca2f70a1ea3bf0deb17ad54a14446de0a300
Author: Ian Barwick
Date: Fri Aug 30 13:30:47 2019 +0900
psql: add tab completion for \df slash commands
"\df" itself was listed for tab completion, but none of the
possible suffixes were.
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index bcc7404c55..bc8a7bd78b 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -1418,7 +1418,8 @@ psql_completion(const char *text, int start, int end)
"\\connect", "\\conninfo", "\\C", "\\cd", "\\copy",
"\\copyright", "\\crosstabview",
"\\d", "\\da", "\\dA", "\\db", "\\dc", "\\dC", "\\dd", "\\ddp", "\\dD",
- "\\des", "\\det", "\\deu", "\\dew", "\\dE", "\\df",
+ "\\des", "\\det", "\\deu", "\\dew", "\\dE",
+ "\\df", "\\dfa", "\\dfn", "\\dfp", "\\dft", "\\dfw",
"\\dF", "\\dFd", "\\dFp", "\\dFt", "\\dg", "\\di", "\\dl", "\\dL",
"\\dm", "\\dn", "\\do", "\\dO", "\\dp", "\\dP", "\\dPi", "\\dPt",
"\\drds", "\\dRs", "\\dRp", "\\ds", "\\dS",
commit 616745c6c2c1f96446cebed7dbadd6f6307c7817
Author: Ian Barwick
Date: Fri Aug 30 13:45:24 2019 +0900
psql: add tab completion for \df slash commands
"\df" itself was listed for tab completion, but none of the
possible suffixes were.
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index d6988f2654..9c8a18ab48 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -870,7 +870,8 @@ psql_completion(const char *text, int start, int end)
static const char *const backslash_commands[] = {
"\\a", "\\connect", "\\conninfo", "\\C", "\\cd", "\\copy", "\\copyright",
- "\\d", "\\da", "\\db", "\\dc", "\\dC", "\\dd", "\\dD", "\\des", "\\det", "\\deu", "\\dew", "\\df",
+ "\\d", "\\da", "\\db", "\\dc", "\\dC", "\\dd", "\\dD", "\\des", "\\det", "\\deu", "\\dew",
+ "\\df", "\\dfa", "\\dfn", "\\dfp", "\\dft", "\\dfw",
"\\dF", "\\dFd", "\\dFp", "\\dFt", "\\dg", "\\di", "\\dl", "\\dL",
"\\dn", "\\do", "\\dp", "\\drds", "\\ds", "\\dS", "\\dt", "\\dT", "\\dv", "\\du", "\\dx",
"\\e", "\\echo", "\\ef", "\\encoding",
commit 8ed98bb4cad91c71f0a04c28428c1e4cd862d938
Author: Ian Barwick
Date: Fri Aug 30 13:41:09 2019 +0900
psql: add tab completion for \df slash commands
"\df" itself was listed for tab completion, but none of the
possible suffixes were.
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index b657ab7b8d..784e2a6bad 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -913,7 +913,8 @@ psql_completion(const char *text, int start, int end)
static const char *const backslash_commands[] = {
"\\a", "\\connect", "\\conninfo", "\\C", "\\cd", "\\copy", "\\copyright",
"\\d", "\\da", "\\db", "\\dc", "\\dC", "\\dd", "\\ddp", "\\dD",
- "\\des", "\\det", "\\deu", "\\dew", "\\dE", "\\df",
+ "\\des", "\\det", "\\deu", "\\dew", "\\dE",
+ "\\df", "\\dfa", "\\dfn", "\\dfp", "\\dft", "\\dfw",
"\\dF", "\\dFd", "\\dFp", "\\dFt", "\\dg", "\\di", "\\dl", "\\dL",
"\\dm", "\\dn", "\\do", "\\dO", "\\dp", "\\drds", "\\ds", "\\dS",
"\\dt", "\\dT", "\\dv", "\\du", "\\dx", "\\dy",
doc: update PL/pgSQL sample loop function
Hi
Here:
https://www.postgresql.org/docs/current/plpgsql-control-structures.html#PLPGSQL-RECORDS-ITERATING
we have a sample PL/PgSQL function (dating from at least 7.2) demonstrating
query result loops, which refreshes some pseudo materialized views stored in
a user-defined table.
As we've had proper materialized views since 9.3, I thought it might
be nice to update this with a self-contained sample which can be used
as-is; see attached patch.
(As a side note the current sample function contains a couple of "%s"
placeholders which should be just "%"; a quick search of plpgsql.sgml
shows this is the only place they occur).
Will submit to the next commitfest.
Regards
Ian Barwick
--
Ian Barwick https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
commit d9e99b90fd0e572b4fd2461d7188a0197dee16df
Author: Ian Barwick
Date: Thu Aug 29 11:49:23 2019 +0900
doc: update PL/pgSQL sample function
The sample PL/PgSQL function here:
https://www.postgresql.org/docs/current/plpgsql-control-structures.html#PLPGSQL-RECORDS-ITERATING
dates from at least PostgreSQL 7.2 and updates pseudo-materialized views
defined in a user table.
Replace it with a more up-to-date example which does the same thing
with actual materialized views, which have been available since
PostgreSQL 9.3
diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml
index ae73630a48..3194173594 100644
--- a/doc/src/sgml/plpgsql.sgml
+++ b/doc/src/sgml/plpgsql.sgml
@@ -2437,19 +2437,29 @@ END LOOP label ;
resulting from the query and the loop body is
executed for each row. Here is an example:
-CREATE FUNCTION cs_refresh_mviews() RETURNS integer AS $$
+CREATE FUNCTION refresh_mviews() RETURNS integer AS $$
DECLARE
mviews RECORD;
BEGIN
-RAISE NOTICE 'Refreshing materialized views...';
-
-FOR mviews IN SELECT * FROM cs_materialized_views ORDER BY sort_key LOOP
+RAISE NOTICE 'Refreshing all materialized views...';
+
+FOR mviews IN
+ SELECT n.nspname AS mv_schema,
+ c.relname AS mv_name,
+ pg_catalog.pg_get_userbyid(c.relowner) AS owner
+ FROM pg_catalog.pg_class c
+LEFT JOIN pg_catalog.pg_namespace n ON (n.oid = c.relnamespace)
+WHERE c.relkind = 'm'
+ ORDER BY 1
+LOOP
--- Now "mviews" has one record from cs_materialized_views
+-- Now "mviews" has one record with information about the materialized view
-RAISE NOTICE 'Refreshing materialized view %s ...', quote_ident(mviews.mv_name);
-EXECUTE format('TRUNCATE TABLE %I', mviews.mv_name);
-EXECUTE format('INSERT INTO %I %s', mviews.mv_name, mviews.mv_query);
+RAISE NOTICE 'Refreshing materialized view %.% (owner: %)...',
+ quote_ident(mviews.mv_schema),
+ quote_ident(mviews.mv_name),
+ quote_ident(mviews.owner);
+EXECUTE format('REFRESH MATERIALIZED VIEW %I.%I', mviews.mv_schema, mviews.mv_name);
END LOOP;
RAISE NOTICE 'Done refreshing materialized views.';
Re: [PATCH] Make configuration file "include" directive handling more robust
On 8/25/19 4:39 AM, Tom Lane wrote: Ian Barwick writes: On 7/17/19 5:34 PM, Kyotaro Horiguchi wrote:> Hello. I don't think this is new to 12. No, though I'm not sure how much this would be seen as a bugfix and how far back it would be sensible to patch. I think this is worth considering as a bugfix; although I'm afraid we can't change the signature of ParseConfigFile/ParseConfigFp in released branches, since extensions could possibly be using those. That limits what we can do --- but it's still possible to detect direct recursion, which seems like enough to produce a nice error message in typical cases. Makes sense. I concur with Kyotaro-san that disallow-empty-include-directives.v1.patch seems a bit brute-force, but where I would put the checks is in ParseConfigFile and ParseConfigDirectory. Also, I don't agree with the goals of prevent-disallowed-includes.patch. I'm utterly not on board with breaking use of "include" in extension files, for instance; while that may not be documented, it works fine, and maybe somebody out there is relying on it. I couldn't for the life of me think of any reason for using it. But if there's undocumented functionality we think someone might be using, shouldn't that be documented somewhere, if only as a note in the code to prevent its accidental removal at a later date? Likewise, while "include" in pg.auto.conf is not really considered supported, I don't see the point of going out of our way to break the historical behavior. The amusing thing about that of course is that the include directive will disappear the next time ALTER SYSTEM is run and the values from the included file will appear in pg.auto.conf, which may cause some headscratching. But I guess hasn't been an actual real-world issue so far. That leads me to propose the attached simplified patch. While I haven't actually tried, I'm pretty sure this should back-patch without trouble. Ah, I see it's been applied already, thanks! Regards Ian Barwick -- Ian Barwick https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: doc: clarify "pg_signal_backend" default role
On 8/28/19 7:04 AM, Tom Lane wrote: Ian Barwick writes: Currently the documentation for the default role "pg_signal_backend" states, somewhat ambiguously, "Send signals to other backends (eg: cancel query, terminate)", giving the impression other signals (e.g. SIGHUP) can be sent too, which is currently not the case. Attached patch clarifies this, adds a descriptive paragraph (similar to what the other default roles have) and a link to the "Server Signaling Functions" section. Pushed with minor tweaking. Thanks! (Note: patches are less likely to fall through the cracks if you add them to the commitfest page.) Yup, though I was intending to add that one together with a couple of related minor doc patches to the next CF. Regards Ian Barwick -- Ian Barwick https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: [PATCH] Tab completion for CREATE OR REPLACE
On Thu, 22 Aug 2019 at 15:05, Wang, Shenhao wrote: > > Hello, hackers: > > I created a patch about tab completion for command CREATE OR REPLACE in psql > includes: > CREATE [ OR REPLACE ] FUNCTION > CREATE [ OR REPLACE ] PROCEDURE > CREATE [ OR REPLACE ] LANGUAGE > CREATE [ OR REPLACE ] RULE name AS ON event > CREATE [ OR REPLACE ] VIEW AS SELECT > CREATE [ OR REPLACE ] AGGREGATE > CREATE [ OR REPLACE ] TRANSFORM > > -- Could you add this to the next commitfest? https://commitfest.postgresql.org/24/ Regards Ian Barwick -- Ian Barwick https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: [PATCH] Stop ALTER SYSTEM from making bad assumptions
On 8/16/19 12:22 AM, Tom Lane wrote: Stephen Frost writes: * Tom Lane (t...@sss.pgh.pa.us) wrote: In hopes of moving this along, I've pushed Ian's last code change, as there seems to be no real argument about that anymore. As for the doc changes, how about the attached revision of what I wrote previously? It gives some passing mention to what ALTER SYSTEM will do, without belaboring it or going into things that are really implementation details. It's certainly better than what we have now. Hearing no other comments, I've pushed that and marked this issue closed. Thanks! Regards Ian Barwick -- Ian Barwick https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: [PATCH] Stop ALTER SYSTEM from making bad assumptions
On 8/6/19 11:16 AM, Stephen Frost wrote: > Greetings, > > * Ian Barwick (ian.barw...@2ndquadrant.com) wrote: >> On 8/6/19 9:52 AM, Stephen Frost wrote:> Greetings, >>> * Tom Lane (t...@sss.pgh.pa.us) wrote: >>>> >>>> + >>>> + External tools might also >>>> + modify postgresql.auto.conf, typically by appending >>>> + new settings to the end. It is not recommended to do this while the >>>> + server is running, since a concurrent ALTER SYSTEM >>>> + command could overwrite such changes. >>>> >>> >>> Alternatively, or maybe also here, we could say "note that appending to >>> the file as a mechanism for setting a new value by an external tool is >>> acceptable even though it will cause duplicates- PostgreSQL will always >>> use the last value set and other tools should as well. Duplicates and >>> comments may be removed when rewriting the file >> >> FWIW, as the file is rewritten each time, *all* comments are removed >> anyway (the first two comment lines in the file with the warning >> are added when the new version of the file is written(). > > Whoah- the file is *not* rewritten each time. It's only rewritten each > time by *ALTER SYSTEM*, but that it not the only thing that's modifying > the file. That mistaken assumption is part of what got us into this > mess... Ah, got it, I thought you were talking about the ALTER SYSTEM behaviour. >>> and parameters may be >>> lower-cased." (istr that last bit being true too but I haven't checked >>> lately). >> >> Ho-hum, they won't be lower-cased, instead currently they just won't be >> overwritten if they're present in pg.auto.conf, which is a slight >> eccentricity, but not actually a problem with the current code as the new value >> will be written last. E.g.: >> >> $ cat postgresql.auto.conf >> # Do not edit this file manually! >> # It will be overwritten by the ALTER SYSTEM command. >> DEFAULT_TABLESPACE = 'space_1' >> >> postgres=# ALTER SYSTEM SET default_tablespace ='pg_default'; >> ALTER SYSTEM >> >> $ cat postgresql.auto.conf >> # Do not edit this file manually! >> # It will be overwritten by the ALTER SYSTEM command. >> DEFAULT_TABLESPACE = 'space_1' >> default_tablespace = 'pg_default' >> >> I don't think that's worth worrying about now. > > Erm, those are duplicates though and we're saying that ALTER SYSTEM > removes those... Seems like we should be normalizing the file to be > consistent in this regard too. True. (Switches brain on)... Ah yes, with the patch previously provided by Tom, it seems to be just a case of replacing "strcmp" with "guc_name_compare" to match the existing string; the name will be rewritten with the value provided to ALTER SYSTEM, which will be normalized to lower case anyway. Tweaked version attached. >> My suggestion for the paragaph in question: >> >> >> External tools which need to write configuration settings (e.g. for replication) >> where it's essential to ensure these are read last (to override versions >> of these settings present in other configuration files), may append settings to >> postgresql.auto.conf. It is not recommended to do this while >> the server is running, since a concurrent ALTER SYSTEM >> command could overwrite such changes. Note that a subsequent >> ALTER SYSTEM will cause postgresql.auto.conf, >> to be rewritten, removing any duplicate versions of the setting altered, and also >> any comment lines present. >> > > I dislike the special-casing of ALTER SYSTEM here, where we're basically > saying that only ALTER SYSTEM is allowed to do this cleanup and that if > such cleanup is wanted then ALTER SYSTEM must be run. This is just saying what ALTER SYSTEM will do, which IMHO we should describe somewhere. Initially when I stated working with pg.auto.conf I had my application append a comment line to show where the entries came from, but not having any idea how pg.auto.conf was modified at that point, I was wondering why the comment subsequently disappeared. Perusing the source code has explained that for me, but would be mighty useful to document that. > What I was trying to get at is a definition of what transformations are > allowed and to make it clear that anything using/modifying the file > needs to be prepared for and work with those transformations. I don't > think we want people assuming that if they don't run ALTER
Re: [PATCH] Stop ALTER SYSTEM from making bad assumptions
al tools which need to write configuration settings (e.g. for replication) where it's essential to ensure these are read last (to override versions of these settings present in other configuration files), may append settings to postgresql.auto.conf. It is not recommended to do this while the server is running, since a concurrent ALTER SYSTEM command could overwrite such changes. Note that a subsequent ALTER SYSTEM will cause postgresql.auto.conf, to be rewritten, removing any duplicate versions of the setting altered, and also any comment lines present. > >> >>The system view >>pg_file_settings >> - can be helpful for pre-testing changes to the configuration file, or for >> + can be helpful for pre-testing changes to the configuration files, or for >>diagnosing problems if a SIGHUP signal did not have the >>desired effects. >> > > This hunk looks fine. > > Reviewing https://www.postgresql.org/docs/current/config-setting.html > again, it looks reasonably comprehensive regarding the format of the > file- perhaps we should link to there from the "external tools might > also modify" para..? "Tool authors should review to understand > the structure of postgresql.auto.conf". This is all on the same page anyway. Regards Ian Barwick -- Ian Barwick https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: [PATCH] Stop ALTER SYSTEM from making bad assumptions
On 8/4/19 1:59 AM, Tom Lane wrote:> Tomas Vondra writes: >> On Fri, Aug 02, 2019 at 06:08:02PM -0700, Andres Freund wrote: >>> We're WAY WAY past feature freeze. This isn't the time to rewrite guc.c, >>> guc-file.l to be suitable for running outside of a backend environment. > >> Right. And even if we had the code, it's not quite backpatchable (which >> we probably should do, considering this is a general ALTER SYSTEM issue, >> so not pg12-only). > >> Not to mention there's no clear consensus this is actually desirable. >> I'd argue forcing external tools (written in arbitrary language) to use >> this library (written in C), just to modify a "stupid" text file is a >> bit overkill. IMO duplicates don't make the file invalid, we should >> handle that correctly/gracefully, so I don't see why external tools >> could not simply append to the file. We can deduplicate the file when >> starting the server, on ALTER SYSTEM, or some other time. > > Yup. I'd also point out that even if we had a command-line tool of this > sort, there would be scenarios where it's not practical or not convenient > to use. We need not go further than "my tool needs to work with existing > PG releases" to think of good examples. I suspect this hasn't been an issue before, simply because until the removal of recovery.conf AFAIK there hasn't been a general use-case where you'd need to modify pg.auto.conf while the server is not running. The use-case which now exists (i.e. for writing replication configuration) is one where the tool will need to be version-aware anyway (like pg_basebackup is), so I don't see that as a particular deal-breaker. But... > I think we should just accept the facts on the ground, which are that > some tools modify pg.auto.conf by appending to it +1. It's just a text file... > and say that that's supported as long as the file doesn't get unreasonably long. Albeit with the caveat that the server should not be running. Not sure how you define "unreasonably long" though. > I'm not at all on board with inventing a requirement for pg.auto.conf > to track its modification history. I don't buy that that's a > widespread need in the first place; if I did buy it, that file > itself is not where to keep the history; and in any case, it'd be > a new feature and it's way too late for v12. Yeah, that's way outside of the scope of this issue. Regards Ian Barwick -- Ian Barwick https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: [PATCH] Stop ALTER SYSTEM from making bad assumptions
On 8/4/19 4:13 AM, Tom Lane wrote: Ian Barwick writes: On 8/3/19 7:27 AM, Tom Lane wrote: Tomas Vondra writes: The main issue however is that no code was written yet. Seems like it ought to be relatively simple ... but I didn't look. The patch I originally sent does exactly this. Ah, you did send a patch, but that tries to maintain the existing behavior of replacing the last occurrence in-place. I think it's simpler and more sensible to just make a sweep to delete all matches, and then append the new setting (if any) at the end, as attached. Yes, that is less convoluted. A more aggressive patch would try to de-duplicate the entire list, not just the current target entry ... but I'm not really excited about doing that in a back-patchable bug fix. I thought about doing that but it's more of a nice-to-have and not essential to fix the issue, as any other duplicate entries will get removed the next time ALTER SYSTEM is run on the entry in question. Maybe as part of a future improvement. I looked at the TAP test you proposed and couldn't quite convince myself that it was worth the trouble. A new test within an existing suite would likely be fine, but a whole new src/test/ subdirectory just for pg.auto.conf seems a bit much. (Note that the buildfarm and possibly the MSVC scripts would have to be taught about each such subdirectory.) Didn't know that, but couldn't find anywhere obvious to put the test. At the same time, we lack any better place to put such a test :-(. Maybe it's time for a "miscellaneous TAP tests" subdirectory? Sounds reasonable. Regards Ian Barwick -- Ian Barwick https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: [PATCH] Stop ALTER SYSTEM from making bad assumptions
On 8/3/19 8:24 AM, Andres Freund wrote: Hi, On 2019-08-03 08:22:29 +0900, Ian Barwick wrote: What I came up with shoehorned a stripped-down version of the backend config parser into fe_utils and provides a function to modify pg.auto.conf in much the same way ALTER SYSTEM does, but with only the basic syntax checking provided by the parser of course. And for completeness a client utility which can be called by scripts etc. I can clean it up and submit it later for reference (got distracted by other things recently) though I don't think it's a particularly good solution due to the lack of actual checks for the provided GUCSs (and the implementation is ugly anyway); something like what Andres suggests below would be far better. I think my main problem with that is that it duplicates a nontrivial amount of code. That is indeed part of the ugliness of the implementation. Regards Ian Barwick -- Ian Barwick https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: [PATCH] Stop ALTER SYSTEM from making bad assumptions
On 8/3/19 7:56 AM, Andres Freund wrote: Hi, On 2019-08-02 18:47:07 -0400, Tom Lane wrote: Stephen Frost writes: I disagree that this should only be addressed in alter system, as I’ve said before and as others have agreed with. Having one set of code that can be used to update parameters in the auto.conf and then have that be used by pg_basebackup, alter system, and external tools, is the right approach. I don't find that to be necessary or even desirable. Many (most?) of the situations where this would be important wouldn't have access to a running backend, and maybe not to any PG code at all --- what if your tool isn't written in C? I think a commandline tool to perform the equivalent of ALTER SYSTEM on a shutdown cluster would be a great tool. It's easy enough to add something with broken syntax, and further down the road such a tool could not only ensure the syntax is correct, but also validate individual settings as much as possible (obviously there's some hairy issues here). What I came up with shoehorned a stripped-down version of the backend config parser into fe_utils and provides a function to modify pg.auto.conf in much the same way ALTER SYSTEM does, but with only the basic syntax checking provided by the parser of course. And for completeness a client utility which can be called by scripts etc. I can clean it up and submit it later for reference (got distracted by other things recently) though I don't think it's a particularly good solution due to the lack of actual checks for the provided GUCSs (and the implementation is ugly anyway); something like what Andres suggests below would be far better. Quite possibly the most realistic way to implement something like that would be a postgres commandline switch, which'd start up far enough to perform GUC checks and execute AlterSystem(), and then shut down again. We already have -C, I think such an option could reasonably be implemented alongside it. Obviously this is widely out of scope for v12. Regards Ian Barwick -- Ian Barwick https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: [PATCH] Stop ALTER SYSTEM from making bad assumptions
On 8/3/19 8:09 AM, Tom Lane wrote: I wrote: Andres Freund writes: I think a commandline tool to perform the equivalent of ALTER SYSTEM on a shutdown cluster would be a great tool. Perhaps, but ... Obviously this is widely out of scope for v12. ... this. Although, there's always echo "alter system set work_mem = 4242;" | postgres --single Maybe we could recommend that to tools that need to do potentially-repetitive modifications? The slight problem with that, particularly with the use-case I am concerned with (writing replication configuration), is: [2019-08-03 08:14:21 JST]FATAL: 0A000: standby mode is not supported by single-user servers (I may be missing something obvious of course) Regards Ian Barwick -- Ian Barwick https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: [PATCH] Stop ALTER SYSTEM from making bad assumptions
On 8/3/19 7:27 AM, Tom Lane wrote: Tomas Vondra writes: There seems to be a consensus that this this not a pg_basebackup issue (i.e. duplicate values don't make the file invalid), and it should be handled in ALTER SYSTEM. Yeah. I doubt pg_basebackup is the only actor that can create such situations. The proposal seems to be to run through the .auto.conf file, remove any duplicates, and append the new entry at the end. That seems reasonable. +1 There was a discussion whether to print warnings about the duplicates. I personally see not much point in doing that - if we consider duplicates to be expected, and if ALTER SYSTEM has the license to rework the config file any way it wants, why warn about it? Personally I agree that warnings are unnecessary. Having played around with the pg.auto.conf stuff for a while, my feeling is that ALTER SYSTEM does indeed have a license to rewrite it (which is what currently happens anyway, with comments and include directives [1] being silently removed) so it seems reasonable to remove duplicate entries and ensure the correct one is processed. [1] suprisingly any include directives present are honoured, which seems crazy to me, see: https://www.postgresql.org/message-id/flat/8c8bcbca-3bd9-dc6e-8986-04a5abdef142%402ndquadrant.com The main issue however is that no code was written yet. Seems like it ought to be relatively simple ... but I didn't look. The patch I originally sent does exactly this. The thread then drifted off into a discussion about providing ways for applications to properly write to pg.auto.conf while PostgreSQL is not running; I have a patch for that which I can submit later (though it is a thing of considerable ugliness). Regards Ian Barwick -- Ian Barwick https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: [PATCH] minor bugfix for pg_basebackup (9.6 ~ )
On 7/27/19 6:52 AM, Alvaro Herrera wrote: On 2019-Jul-25, Michael Paquier wrote: On Wed, Jul 24, 2019 at 11:23:30AM -0400, Alvaro Herrera wrote: Heh, yesterday I revised the original patch as attached and was about to push when the bell rang. I like this one because it keeps the comment to one line and it mentions the function name in charge of the validation (useful for grepping later on). It's a bit laconic because of the long function name and the desire to keep it to one line, but it seems sufficient to me. Looks fine to me. A nit: addition of braces for the if block. Even if that a one-liner, there is a comment so I think that this makes the code more readable. Yeah, I had removed those on purpose, but that was probably inconsistent with my own reviews of others' patches. I pushed it with them. Thanks Regards Ian Barwick -- Ian Barwick https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: [PATCH] minor bugfix for pg_basebackup (9.6 ~ )
On 7/23/19 5:10 PM, Michael Paquier wrote:
On Mon, Jul 22, 2019 at 12:58:40PM -0400, Alvaro Herrera wrote:
Maybe it's just me, but it seems weird to try to forestall a problem
that cannot occur by definition. I would rather remove the escaping,
and add a one-line comment explaining why we don't do it?
No objections with doing that either, really. Perhaps you would
prefer pushing a patch among those lines by yourself?
One argument that I got in mind to justify the escaping would be if we
add a new feature in pg_basebackup to write a new set of recovery
options on an existing data folder, which does not require an option.
In this case, if the escaping does not exist, starting the server
would fail with a confusing parsing error if a quote is added to the
slot name. But if the escaping is done, then we would get a correct
error that the replication slot value includes an incorrect character.
If such an hypothetical option is added, most likely this would be
noticed anyway, so that's mainly nannyism from my side.
It'd be better if such a hypothetical option validated the provided
slot name anwyay, to prevent later surprises.
Revised patch attached, which as Alvaro suggests removes the escaping
and adds a comment explaining why the raw value can be passed as-is.
Regards
Ian Barwick
--
Ian Barwick https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
new file mode 100644
index 77a7c14..9c910cb
*** a/src/bin/pg_basebackup/pg_basebackup.c
--- b/src/bin/pg_basebackup/pg_basebackup.c
*** GenerateRecoveryConf(PGconn *conn)
*** 1716,1724
if (replication_slot)
{
! escaped = escape_quotes(replication_slot);
appendPQExpBuffer(recoveryconfcontents, "primary_slot_name = '%s'\n", replication_slot);
- free(escaped);
}
if (PQExpBufferBroken(recoveryconfcontents) ||
--- 1716,1727
if (replication_slot)
{
! /*
! * The slot name does not need escaping as it can only consist of [a-zA-Z0-9_].
! * The validity of the name has already been proven, as a slot must have been
! * successfully created with that name to reach this point.
! */
appendPQExpBuffer(recoveryconfcontents, "primary_slot_name = '%s'\n", replication_slot);
}
if (PQExpBufferBroken(recoveryconfcontents) ||
Re: [PATCH] minor bugfix for pg_basebackup (9.6 ~ )
On 7/19/19 7:45 PM, Sergei Kornilov wrote: Hi Oh. Replication slot name currently can contains only a-z0-9_ characters. So we can not actually write such recovery.conf, pg_basebackup will stop before. But perform escape_quotes on string and not use result - error anyway. Good point, it does actually fail with an error if an impossible slot name is provided, so the escaping is superfluous anyway. I'll take another look at it later as it's not exactly critical, just stuck out when I was passing through the code. Regards Ian Barwick -- Ian Barwick https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
[PATCH] minor bugfix for pg_basebackup (9.6 ~ )
Hi In pg_basebackup's GenerateRecoveryConf() function, the value for "primary_slot_name" is escaped, but the original, non-escaped value is written. See attached patch. This has been present since the code was added in 9.6 (commit 0dc848b0314). Regards Ian Barwick -- Ian Barwick https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services pg_basebackup-generate-recovery-conf.v1.patch Description: Binary data
Re: Documentation fix for adding a column with a default value
On 7/19/19 12:51 AM, Daniel Gustafsson wrote: On 18 Jul 2019, at 17:46, Daniel Westermann (DWE) wrote: The suggested change pares down the "Tip" to more of a brief "Note", which IMHO is a bit terse for that section of the documentation (which has more of a tutorial character), and the contents of the original tip basically still apply for volatile default values anyway. I've attached another suggestion for rewording this which should also make the mechanics of the operation a little clearer. Thank you, that better explains it. Looks good to me. Shouldn't we add that to the current commit fest? The current commitfest is closed for new additions, but please add it to the next one (2019-09) and it will be picked up then. To me it looks like a minor documentation correction to fix an omission from a patch already in PostgreSQL. Regards Ian Barwick -- Ian Barwick https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: [PATCH] Make configuration file "include" directive handling more robust
On 7/17/19 5:34 PM, Kyotaro Horiguchi wrote:> Hello.
>
> At Wed, 17 Jul 2019 12:29:43 +0900, Ian Barwick wrote
in <8c8bcbca-3bd9-dc6e-8986-04a5abdef...@2ndquadrant.com>
>> Hi
>>
>> While poking about with [1], I noticed a few potential issues with the
>> inclusion handling for configuration files; another issue is
>> demonstrated in [2].
>>
>> [1]
>>
https://www.postgresql.org/message-id/aed6cc9f-98f3-2693-ac81-52bb0052307e%402ndquadrant.com
>> ("Stop ALTER SYSTEM from making bad assumptions")
>> [2]
>>
https://www.postgresql.org/message-id/CY4PR1301MB22001D3FAAB3499C5D41DE23A9E50%40CY4PR1301MB2200.namprd13.prod.outlook.com
>> ("First Time Starting Up PostgreSQL and Having Problems")
>
> Yeah! That's annoying..
>
>> Specifically these are:
>>
>> 1) Provision of empty include directives
>> =
>>
>> The default postgresql.conf file includes these thusly:
>>
>> #include_dir = '' # include files ending in '.conf' from
>> # a directory, e.g., 'conf.d'
>> #include_if_exists = '' # include file only if it exists
>> #include = '' # include file
>>
>> Currently, uncommenting them but leaving the value empty (as happened
>> in [2] above) can
>> result in unexpected behaviour.
>>
>> For "include" and "include_if_exists", it's a not critical issue as
>> non-existent
>> files are, well, non-existent, however this will leave behind the
>> cryptic
>> message "input in flex scanner failed" in pg_file_settings's "error"
>> column, e.g.:
>>
>> postgres=# SELECT sourceline, seqno, name, setting, applied, error
>> FROM pg_file_settings
>> WHERE error IS NOT NULL;
>> sourceline | seqno | name | setting | applied |error
>>
+---+--+-+-+--
>>1 | 45 | | | f | input in flex scanner failed
>>1 | 46 | | | f | input in flex scanner failed
>> (2 rows)
>>
>> However, an empty value for "include_dir" will result in the current
>> configuration
>> file's directory being read, which can result in circular inclusion
>> and triggering
>> of the nesting depth check.
>>
>> Patch {1} makes provision of an empty value for any of these
>> directives cause
>> configuration file processing to report an approprate error, e.g.:
>>
>> postgres=# SELECT sourceline, seqno, name, setting, applied, error
>> FROM pg_file_settings
>> WHERE error IS NOT NULL;
>> sourceline | seqno | name | setting | applied | error
>>
+---+--+-+-+---
>> 757 | 45 | | | f | "include" must not be empty
>> 758 | 46 | | | f | "include_if_exists" must not be empty
>> 759 | 47 | | | f | "include_dir" must not be empty
>
> The patch 1 looks somewhat superficial. All the problems are
> reduced to creating unexpected filepath for
> inclusion. AbsoluteConfigLocation does the core work, and it can
> issue generic error message covering all the cases like:
>
> invalid parameter "" at :
>
> which seems sufficient. (The function needs some additional
> parameters.)
That seems unnecessarily complex to me, as we'd be overloading a
function with a single purpose (to manipulate a path) with some
of the parsing logic/control.
>
>> 2) Circular inclusion of configuration files
>>
>>
>> Currently there is a simple maximum nesting threshold (currently 10)
>> which
>> will stop runaway circular inclusions. However, if triggered, it's not
>> always obvious what triggered it, and sometimes resource exhaustion
>> might kick in beforehand (as appeared to be the case in [2] above).
>>
>> Patch {2} attempts to handle this situation by keeping track of which
>> files have already been included (based on their absolute, canonical
>> path) and reporting an error if they were encountered again.
>
> This seems to me to be overkill. The issue [2] is prevented by
> the patch 1's amendment.
Yes, that particular issue is prevented, but this patch is intended to
provide better protection against explicit circular inclusions, e.g.
if someone adds "include 'postgresql.conf'"
Re: Fw: Documentation fix for adding a column with a default value
On Wed, 17 Jul 2019 at 15:42, Daniel Westermann (DWE) < daniel.westerm...@dbi-services.com> wrote: > >__ > >From: Daniel Westermann (DWE) > >Sent: Monday, July 15, 2019 13:01 > >To: pgsql-hack...@postgresql.org > >Subject: Documentation fix for adding a column with a default value > > > >Hi, > > > >the tip in the "Adding a column" section is not true anymore since > PostgreSQL 11: > > > > >https://www.postgresql.org/docs/current/ddl-alter.html#DDL-ALTER-ADDING-A-COLUMN > <https://www.postgresql.org/docs/current/ddl-alter.html#DDL-ALTER-ADDING-A-COLUMN> > > > >Attached a patch proposal for this. > > Seems the first mail didn't make it ... > Actually it did, I was about to reply to it :) The suggested change pares down the "Tip" to more of a brief "Note", which IMHO is a bit terse for that section of the documentation (which has more of a tutorial character), and the contents of the original tip basically still apply for volatile default values anyway. I've attached another suggestion for rewording this which should also make the mechanics of the operation a little clearer. Regards Ian Barwick -- Ian Barwick https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services doc-alter-table-default-value-pg11.v1.patch Description: Binary data
[PATCH] Make configuration file "include" directive handling more robust
s on patch {2}).
Extension example:
postgres=# CREATE EXTENSION repmgr;
ERROR: "include" not permitted in file
"/home/barwick/devel/postgres/builds/HEAD/share/extension/repmgr.control" line 8
postgres=# CREATE EXTENSION repmgr;
ERROR: "include_dir" not permitted in file
"/home/barwick/devel/postgres/builds/HEAD/share/extension/repmgr.control" line 9
postgres=# CREATE EXTENSION repmgr;
ERROR: "include_if_exists" not permitted in file
"/home/barwick/devel/postgres/builds/HEAD/share/extension/repmgr.control" line 10
pg.auto.conf example:
postgres=# ALTER SYSTEM SET default_tablespace ='pg_default';
ERROR: could not parse contents of file "postgresql.auto.conf"
postgres=# SELECT regexp_replace(sourcefile, '^/.+/','') AS sourcefile,
seqno, name, setting, applied, error
FROM pg_file_settings WHERE error IS NOT NULL;
sourcefile | seqno | name | setting | applied | error
--+---+--+-+-+-
postgresql.auto.conf |45 | | | f | "include" not
permitted
(1 row)
The patch also has the side-effect that "include" directives are no longer
(silently) removed from "postgresql.auto.conf"; as the only way they can get
into the file in the first place is by manually editing it, I feel it's
reasonable for the user to be made aware that they're not valid and have to
manually remove them.
Patches
===
Code:
{1} disallow-empty-include-directives.v1.patch
{2} track-included-files.v1.patch
{3} prevent-disallowed-includes.v1.patch
TAP tests:
{1} tap-test-configuration.v1.patch
{2} tap-test-disallow-empty-include-directives.v1.patch
{3} tap-test-track-included-files.v1.patch
{4} tap-test-prevent-disallowed-includes.v1.patch
Patches apply cleanly to REL_12_STABLE/HEAD, they could be modfied for
all supported versions if required. I can consolidate the patches
if preferred.
Regards
Ian Barwick
--
Ian Barwick https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
commit c46b50d2a1aa81e5457a61a5b3814e5a560e68d6
Author: Ian Barwick
Date: Tue Jul 16 00:01:20 2019 +0900
Explicitly disallow empty "include" directives
For "include" and "include_if_exists", provision of an empty value
resulted in a rather cryptic "input in flex scanner failed" error being
reported.
For "include_dir", provision of an empty value would result in all
configuration files in the same directory as the current file being
parsed, probably resulting in circular inclusion.
diff --git a/src/backend/utils/misc/guc-file.l b/src/backend/utils/misc/guc-file.l
index 1c8b5f7d84..807d950291 100644
--- a/src/backend/utils/misc/guc-file.l
+++ b/src/backend/utils/misc/guc-file.l
@@ -782,10 +782,17 @@ ParseConfigFp(FILE *fp, const char *config_file, int depth, int elevel,
* An include_dir directive isn't a variable and should be
* processed immediately.
*/
- if (!ParseConfigDirectory(opt_value,
- config_file, ConfigFileLineno - 1,
- depth + 1, elevel,
- head_p, tail_p))
+ if (opt_value[0] == '\0')
+ {
+record_config_file_error("\"include_dir\" must not be empty",
+ config_file, ConfigFileLineno - 1,
+ head_p, tail_p);
+OK = false;
+ }
+ else if (!ParseConfigDirectory(opt_value,
+ config_file, ConfigFileLineno - 1,
+ depth + 1, elevel,
+ head_p, tail_p))
OK = false;
yy_switch_to_buffer(lex_buffer);
pfree(opt_name);
@@ -797,7 +804,14 @@ ParseConfigFp(FILE *fp, const char *config_file, int depth, int elevel,
* An include_if_exists directive isn't a variable and should be
* processed immediately.
*/
- if (!ParseConfigFile(opt_value, false,
+ if (opt_value[0] == '\0')
+ {
+record_config_file_error("\"include_if_exists\" must not be empty",
+ config_file, ConfigFileLineno - 1,
+ head_p, tail_p);
+OK = false;
+ }
+ else if (!ParseConfigFile(opt_value, false,
config_file, ConfigFileLineno - 1,
depth + 1, elevel,
head_p, tail_p))
@@ -812,7 +826,14 @@ ParseConfigFp(FILE *fp, const char *config_file, int depth, int elevel,
* An include directive isn't a variable and should be processed
* immediately.
*/
- if (!ParseConfigFile(opt_value, true,
+ if (opt_value[0] == '\0')
+ {
+record_config_file_error("\"include\" must not be empty",
+ config_file, ConfigFileLineno - 1,
+ head_p, tail_p);
+OK = false;
+ }
+ else if (!ParseConfigFile(opt_value, true,
config_file, ConfigFileL
Re: doc: mention pg_reload_conf() in pg_hba.conf documentation
On 7/16/19 10:09 AM, Bruce Momjian wrote: On Mon, Jul 15, 2019 at 12:47:18PM +0900, Ian Barwick wrote: Hi I noticed the documentation for pg_hba.conf: https://www.postgresql.org/docs/current/auth-pg-hba-conf.html says: you will need to signal the postmaster (using pg_ctl reload or kill -HUP) to make it re-read the file. It would be useful to mention pg_reload_conf() as another option here, as done elsewhere in the docs. Patch with suggested change attached. Oh, good point. Not sure how we missed that, but I had to fix a mention in pg_hba.conf a while ago too. Also, there were two mentions in that file, so I fixed them both with the attached patch. Backpatched to 9.4. Thanks! I only noticed it because I was writing up some basic instructions for someone not very familiar with Pg, and cross-referencing to the documentation. Regards Ian Barwick -- Ian Barwick https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
doc: mention pg_reload_conf() in pg_hba.conf documentation
Hi I noticed the documentation for pg_hba.conf: https://www.postgresql.org/docs/current/auth-pg-hba-conf.html says: you will need to signal the postmaster (using pg_ctl reload or kill -HUP) to make it re-read the file. It would be useful to mention pg_reload_conf() as another option here, as done elsewhere in the docs. Patch with suggested change attached. Regards Ian Barwick -- Ian Barwick https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services diff --git a/doc/src/sgml/client-auth.sgml b/doc/src/sgml/client-auth.sgml new file mode 100644 index 3ed74d8..c90ca0b *** a/doc/src/sgml/client-auth.sgml --- b/doc/src/sgml/client-auth.sgml *** hostnogssenc databaseSIGHUPSIGHUP signal. If you edit the file on an active system, you will need to signal the postmaster !(using pg_ctl reload or kill -HUP) to make it !re-read the file. --- 650,658 SIGHUPSIGHUP signal. If you edit the file on an active system, you will need to signal the postmaster !(using pg_ctl reload, kill -HUP !or by calling the SQL function pg_reload_conf()) !to make it re-read the file.
Re: [PATCH] Implement uuid_version()
On 7/14/19 9:40 PM, Peter Eisentraut wrote: On 2019-07-13 17:13, Fabien COELHO wrote: What about avoiding a redirection with something like: Datum (* const pg_random_uuid)(PG_FUNCTION_ARGS) = gen_random_uuid; That seems very confusing. Dunno. Possibly. The user does not have to look at the implementation, and probably such code would deserve a comment. The point is to avoid one call so as to perform the same (otherwise the pg_random_uuid would be slightly slower), and to ensure that it behaves the same, as it would be the very same function by construction. I've switched the patch to ready anyway. committed Small doc tweak suggestion - the pgcrypto docs [1] now say about gen_random_uuid(): Returns a version 4 (random) UUID. (Obsolete, this function is now also included in core PostgreSQL.) which gives the impression the code contains two versions of this function, the core one and an obsolete one in pgcrypto. Per the commit message the situation is actually: The pgcrypto implementation now internally redirects to the built-in one. Suggested wording improvement in the attached patch. [1] https://www.postgresql.org/docs/devel/pgcrypto.html#id-1.11.7.34.9 Regards Ian Barwick -- Ian Barwick https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services diff --git a/doc/src/sgml/pgcrypto.sgml b/doc/src/sgml/pgcrypto.sgml new file mode 100644 index 0acd11e..f636703 *** a/doc/src/sgml/pgcrypto.sgml --- b/doc/src/sgml/pgcrypto.sgml *** gen_random_bytes(count integer) returns *** 1132,1139 gen_random_uuid() returns uuid !Returns a version 4 (random) UUID. (Obsolete, this function is now also !included in core PostgreSQL.) --- 1132,1141 gen_random_uuid() returns uuid !Returns a version 4 (random) UUID. (This function now redirects internally !to the core PostgreSQL function providing !the same functionality and is included in the extension for backwards compatibility; !see for details.)
Re: doc: minor update for description of "pg_roles" view
On 7/11/19 3:24 AM, Bruce Momjian wrote: On Wed, Jul 10, 2019 at 02:35:56PM +0900, Ian Barwick wrote: Hi Here: https://www.postgresql.org/docs/12/view-pg-roles.html we state: "This view explicitly exposes the OID column of the underlying table, since that is needed to do joins to other catalogs." I think it's superfluous to mention this now OIDs are exposed by default; attached patch (for REL_12_STABLE and HEAD) simply removes this sentence. Patch applied though PG 12. Thanks. Thanks! Regards Ian Barwick -- Ian Barwick https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
doc: minor update for description of "pg_roles" view
Hi Here: https://www.postgresql.org/docs/12/view-pg-roles.html we state: "This view explicitly exposes the OID column of the underlying table, since that is needed to do joins to other catalogs." I think it's superfluous to mention this now OIDs are exposed by default; attached patch (for REL_12_STABLE and HEAD) simply removes this sentence. Regards Ian Barwick -- Ian Barwick https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml new file mode 100644 index 3428a7c..68ad507 *** a/doc/src/sgml/catalogs.sgml --- b/doc/src/sgml/catalogs.sgml *** SELECT * FROM pg_locks pl LEFT JOIN pg_p *** 9995,10005 that blanks out the password field. - -This view explicitly exposes the OID column of the underlying table, -since that is needed to do joins to other catalogs. - - pg_roles Columns --- 9995,1
Re: [PATCH] Stop ALTER SYSTEM from making bad assumptions
> In particular, in order to consider it unexpected, you have to suppose >> that the content rules for postgresql.auto.conf are different from those >> for postgresql.conf (wherein we clearly allow last-one-wins). Can you >> point to any user-facing documentation that says that? > > The backend and frontend tools don't modify postgresql.conf, and we > don't document how to modify postgresql.auto.conf at *all*, even though > we clearly now expect tool authors to go modifying it so that they can > provide the same capabilities that pg_basebackup does and which they > used to through recovery.conf, so I don't really see that as being > comparable. > > The only thing we used to have to go on was what ALTER SYSTEM did, and > then pg_basebackup went and did something different, and enough so that > they ended up conflicting with each other, leading to this discussion. Or looking at it from another perspective - previously there was no particular use-case for appending to .auto.conf, until it (implicitly) became the only way of doing what recovery.conf used to do, and happened to expose the issue at hand. Leaving aside pg_basebackup and the whole issue of writing replication configuration, .auto.conf remains a text file which could potentially include duplicate entries, no matter how much we stipulate it shouldn't. As-is, ALTER SYSTEM fails to deal with this case, which in my opinion is a bug and a potential footgun which needs fixing. (Though we'll still need to define/provide a way of writing configuration while the server is not running, which will be guaranteed to be read in last when it starts up). Regards Ian Barwick -- Ian Barwick https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: [PATCH] Stop ALTER SYSTEM from making bad assumptions
On 6/25/19 4:06 AM, Alvaro Herrera wrote: > On 2019-Jun-24, Robert Haas wrote: > >> On Sat, Jun 22, 2019 at 5:13 PM Stephen Frost wrote: >>> All that said, whatever code it is that we write for pg_basebackup to >>> do this properly should go into our client side library, so other tools >>> can leverage that and avoid having to write it themselves. >> >> That is probably only going to help people who are writing in C (or >> maybe some close family member) and a lot of tools for managing >> PostgreSQL will be written in scripting languages. > > But we already have ALTER SYSTEM, so why do we need to write it again? > You just need to check whether the system is running: if it is, connect > and do "ALTER SYSTEM". If it isn't, do `echo ALTER SYSTEM | postgres > --single`. Maybe we can embed smarts to do that in, say, pg_ctl; then > everybody has access to it. Unfortunately, to quote the emitted log message, "standby mode is not supported by single-user servers", which as-is renders this approach useless for setting up replication configuration on a standby server (unless I'm missing something). I've looked in to what might be involved into creating a client-side function for modifying .auto.conf while the system is not running, and basically it seems to involve maintaining a stripped down version of ParseConfigFp() which doesn't recurse (because we don't allow "include" directives in .auto.conf, right? Right? I'll send in a different patch for that later...) and somehow exposing write_auto_conf_file(). And for all those scripts which can't call the putative frontend C function, we could provide a utility called "pg_alter_system" or similar which accepts a name and a value and (provided the system is not running) "safely" writes it to .auto.conf (though of course it won't be able to validate the provided parameter(s)). Alternatively (waves hand vaguely in air) there might be some way of creating a single user startup mode for the express purpose of leveraging the backend code to modify .auto.conf. Bur that seems like a lot of effort and complexity to replace what, in Pg11 and earlier, was just a case of writing to recovery.conf. Which brings me to another thought which AFAIK hasn't been discussed - what use-cases are there for modifying .auto.conf when the system isn't running? The only one I can think of is the case at hand, i.e. configuring replication after cloning a standby in a manner which *guarantees* that the replication configuration will be read at startup, which was the case with recovery.conf in Pg11 and earlier. For anything else, it seems reasonable to me to expect any customised settings to be written (e.g. by a provisioning system) to the normal configuration file(s). Having pg_basebackup write the replication configuration to a normal file is icky because there's no guarantee the configuration will be written last, or even included at all, which is a regression against earlier versions as there you could clone a standby and (assuming there are no issues with any cloned configuration files) have the standby start up reliably. Regards Ian Barwick -- Ian Barwick https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: doc: update "relam" description in pg_class catalog reference
On 6/20/19 1:07 PM, Michael Paquier wrote: On Thu, Jun 20, 2019 at 11:20:46AM +0900, Ian Barwick wrote: Whoops, correct version attached. Sorry about the noise. v2 looks fine to me, committed. Thanks! Thanks! Regards Ian Barwick -- Ian Barwick https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: doc: update "relam" description in pg_class catalog reference
On 6/20/19 11:17 AM, Ian Barwick wrote: Hi Here: https://www.postgresql.org/docs/devel/catalog-pg-class.html the description for "relam" has not been updated to take into account table access methods; patch attached. Whoops, correct version attached. Sorry about the noise. Regards Ian Barwick -- Ian Barwick https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml new file mode 100644 index 1300c7b..84f730c *** a/doc/src/sgml/catalogs.sgml --- b/doc/src/sgml/catalogs.sgml *** SCRAM-SHA-256$iteration *** 1730,1736 relam oid pg_am.oid ! If this is an index, the access method used (B-tree, hash, etc.) --- 1730,1736 relam oid pg_am.oid ! If this is a table or an index, the access method used (heap, B-tree, hash, etc.)
doc: update "relam" description in pg_class catalog reference
Hi Here: https://www.postgresql.org/docs/devel/catalog-pg-class.html the description for "relam" has not been updated to take into account table access methods; patch attached. Regards Ian Barwick -- Ian Barwick https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml new file mode 100644 index 1300c7b..df68cf7 *** a/doc/src/sgml/catalogs.sgml --- b/doc/src/sgml/catalogs.sgml *** SCRAM-SHA-256$iteration *** 1730,1736 relam oid pg_am.oid ! If this is an index, the access method used (B-tree, hash, etc.) --- 1730,1736 relam oid pg_am.oid ! If this is table or an index, the access method used (heap, B-tree, hash, etc.)
Re: [PATCH] Stop ALTER SYSTEM from making bad assumptions
n 6/18/19 12:41 AM, Stephen Frost wrote:
> Greetings,
>
> * Ian Barwick (ian.barw...@2ndquadrant.com) wrote
(...)
>> I suggest explicitly documenting postgresql.auto.conf behaviour (and the
circumstances
>> where it's acceptable to modify it outside of ALTER SYSTEM calls) in the
documentation
>> (and possibly in the code), so anyone writing utilities which need to
>> append to postgresql.auto.conf knows what the situation is.
>
> Yeah, I would think that, ideally, we'd have some code in the common
> library that other utilities could leverage and which the backend would
> also use.
So maybe something along the lines of creating a stripped-down variant of
AlterSystemSetConfigFile() (from "src/backend/utils/misc/guc.c") which can be
called from front-end code to safely modify .auto.conf while the server is *not*
running.
I'm not terribly familiar with the GUC code, but that would presumably mean
making
parts of the GUC parsing/handling code linkable externally (ParseConfigFp()
etc.)
as you'd need to parse the file before rewriting it. Something like (minimal
pseudo-code):
void
alter_system_set(char *name, char *value)
{
/*
* check here that the server is *not* running
*/
...
ParseConfigFp(infile, AutoConfFileName, 0, LOG, , )
...
/*
* some robust portable way of ensuring another process can't
* modify the file(s) until we're done
*/
lock_file(AutoConfFileName);
replace_auto_config_value(, , name, value);
write_auto_conf_file(AutoConfTmpFileName, head)
durable_rename(AutoConfTmpFileName, AutoConfFileName, ERROR);
FreeConfigVariables(head);
unlock_file(AutoConfFileName);
}
I'm not sure how feasible it is to validate the provided parameter
without the server running, but if not possible, that's not any worse than the
status quo, i.e. the utility has to be trusted to write the correct parameters
to the file anyway.
The question is though - is this a change which is practical to make at this
point
in the release cycle for Pg12?
Regards
Ian Barwick
--
Ian Barwick https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
Re: [PATCH] Stop ALTER SYSTEM from making bad assumptions
On 6/19/19 12:46 PM, Amit Kapila wrote: On Mon, Jun 17, 2019 at 8:20 PM Ian Barwick wrote: On 6/15/19 1:08 AM, Stephen Frost wrote: > * Amit Kapila (amit.kapil...@gmail.com) wrote: >> Right. I think if possible, it should use existing infrastructure to >> write to postgresql.auto.conf rather than inventing a new way to >> change it. Apart from this issue, if we support multiple ways to edit >> postgresql.auto.conf, we might end up with more problems like this in >> the future where one system is not aware of the way file being edited >> by another system. > > I agere that there should have been some effort put into making the way > ALTER SYSTEM is modified be consistent between the backend and utilities > like pg_basebackup (which would also help third party tools understand > how a non-backend application should be modifying the file). Did you mean to say "the way postgresql.auto.conf is modified"? Yes, that is what we are discussing here. I think what we can do here is to extract the functionality to set the parameter in .auto.conf from AlterSystemSetConfigFile and expose it via a function that takes (option_name, value) as a parameter. Yup, I was just considering what's involved there, will reply to another mail in the thread on that. Then we can expose it via some SQL function like set_auto_config (similar to what we have now for set_config/set_config_by_name). I think if we have something like that then pg_basebackup or any other utility can use it in a consistent way. Umm, but the point is here, the server will *not* be running at this point, so calling an SQL function is out of the question. And if the server is running, then you just execute "ALTER SYSTEM". Regards Ian Barwick -- Ian Barwick https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: [PATCH] Stop ALTER SYSTEM from making bad assumptions
On 6/17/19 2:58 AM, Magnus Hagander wrote: On Sun, Jun 16, 2019 at 7:43 PM Stephen Frost mailto:sfr...@snowman.net>> wrote: * Tom Lane (t...@sss.pgh.pa.us <mailto:t...@sss.pgh.pa.us>) wrote: > Stephen Frost mailto:sfr...@snowman.net>> writes: > > what we should do is clean them up (and possibly > > throw a WARNING or similar at the user saying "something modified your > > postgresql.auto.conf in an unexpected way"). I'd suggest we do that on > > every ALTER SYSTEM call. > > +1 for having ALTER SYSTEM clean out duplicates. Not sure whether > a WARNING would seem too in-your-face. I'd hope for a warning from basically every part of the system when it detects, clearly, that a file was changed in a way that it shouldn't have been. If we don't throw a warning, then we're implying that it's acceptable, but then cleaning up the duplicates, which seems pretty confusing. > +1. Silently "fixing" the file by cleaning up duplicates is going to be even > more confusing o uses who had seen them be there before. Some sort of notification is definitely appropriate here. However, going back to the original scenario (cascaded standby set up using "pg_basebackup --write-recovery-conf") there would now be a warning emitted the first time anyone executes ALTER SYSTEM (about duplicate "primary_conninfo" entries) which would not have occured in Pg11 and earlier (and which will no doubt cause consternation along the lines "how did my postgresql.auto.conf get modified in an unexpected way? OMG? Bug? Was I hacked?"). Regards Ian Barwick -- Ian Barwick https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: [PATCH] Stop ALTER SYSTEM from making bad assumptions
Hi On 6/15/19 1:08 AM, Stephen Frost wrote: > Greetings, > > * Ian Barwick (ian.barw...@2ndquadrant.com) wrote: >> Consider the following cascading standby setup with PostgreSQL 12: >> >> - there exists a running primary "A" >> - standby "B" is cloned from primary "A" using "pg_basebackup --write-recovery-conf" >> - standby "C" is cloned from standby "B" using "pg_basebackup --write-recovery-conf" (...) >> However, executing "ALTER SYSTEM SET primary_conninfo = 'host=someothernode'" left >> standby "C"'s "postgresql.auto.conf" file looking like this: >> >># Do not edit this file manually! >># It will be overwritten by the ALTER SYSTEM command. >>primary_conninfo = 'host=someothernode' >>primary_conninfo = 'host=node_B' >> >> which seems somewhat broken, to say the least. > > Yes, it's completely broken, which I've complained about at least twice > on this list to no avail. > > Thanks for putting together an example case pointing out why it's a > serious issue. The right thing to do here it so create an open item for > PG12 around this. Done. >> Attached patch attempts to rectify this situation by having replace_auto_config_value() >> deleting any duplicate entries first, before making any changes to the last entry. > > While this might be a good belt-and-suspenders kind of change to > include, I don't think pg_basebackup should be causing us to have > multiple entries in the file in the first place.. (...) >> Also attached is a set of TAP tests to check ALTER SYSTEM works as expected (or >> at least as seems correct to me). > > In my view, at least, we should have a similar test for pg_basebackup to > make sure that it doesn't create an invalid .auto.conf file. Indeed... I'd be happy to create tests... but first we need a definition of what constitutes a valid .auto.conf file. If that definition includes requiring that a parameter may occur only once, then we need to provide a way for utilities such as pg_basebackup to write the replication configuration to a configuration file in such a way that it doesn't somehow render that file invalid. In Pg11 and earlier, it was just a case of writing (or overwriting) recovery.conf. In Pg12, the code in pg_basebackup implies the correct thing to do is append to .auto.conf, but as demonstrated that can cause problems with duplicate entries. Having pg_basebackup, or any other utility which clones a standby, parse the file itself to remove duplicates seems like a recipe for pain and badly duplicated effort (unless there's some way of calling the configuration parsing code while the server is not running). We could declare that the .auto.conf file will be reset to the default state when a standby is cloned, but the implicit behaviour so far has been to copy the file as-is (as would happen with any other configuration files in the data directory). We could avoid the need for modifying the .auto.conf file by declaring that a configuration with a specific name in the data directory (let's call it "recovery.conf" or "replication.conf") can be used by any utilities writing replication configuration (though of course in contrast to the old recovery.conf it would be included, if exists, as a normal configuration file, though then the precedence would need to be defined, etc..). I'm not sure off the top of my head whether something like that has already been discussed, though it's presumably a bit late in the release cycle to make such changes anyway? >>> This is absolutely the fault of the system for putting in multiple >>> entries into the auto.conf, which it wasn't ever written to handle. >> > * Amit Kapila (amit.kapil...@gmail.com) wrote: >> Right. I think if possible, it should use existing infrastructure to >> write to postgresql.auto.conf rather than inventing a new way to >> change it. Apart from this issue, if we support multiple ways to edit >> postgresql.auto.conf, we might end up with more problems like this in >> the future where one system is not aware of the way file being edited >> by another system. > > I agere that there should have been some effort put into making the way > ALTER SYSTEM is modified be consistent between the backend and utilities > like pg_basebackup (which would also help third party tools understand > how a non-backend application should be modifying the file). Did you mean to say "the way postgresql.auto.conf is modified"? I suggest explicitly documenting postgresql.auto.conf behaviour (and the circumstances where it's acceptable to modify it outside of ALTER SYSTEM calls) in the documentation (and possibly in
[PATCH] Stop ALTER SYSTEM from making bad assumptions
Hi
Consider the following cascading standby setup with PostgreSQL 12:
- there exists a running primary "A"
- standby "B" is cloned from primary "A" using "pg_basebackup
--write-recovery-conf"
- standby "C" is cloned from standby "B" using "pg_basebackup
--write-recovery-conf"
So far, so good, everything works as expected.
Now, for whatever reason, the user wishes standby "C" to follow another upstream
node (which one is not important here), so the user, in the comfort of their
own psql
command line (no more pesky recovery.conf editing!) issues the following:
ALTER SYSTEM SET primary_conninfo = 'host=someothernode';
and restarts the instance, and... it stays connected to the original upstream
node.
Which is unexpected.
Examining the the restarted instance, "SHOW primary_conninfo" still displays
the original value for "primary_conninfo". Mysteriouser and mysteriouser.
What has happened here is that with the option "--write-recovery-conf", Pg12's
pg_basebackup (correctly IMHO) appends the recovery settings to
"postgresql.auto.conf".
However, on standby "C", pg_basebackup has dutifully copied over standby "B"'s
existing "postgresql.auto.conf", which already contains standby "B"'s
replication configuration, and appended standby "C"'s replication configuration
to that, which (before "ALTER SYSTEM" was invoked) looked something like this:
# Do not edit this file manually!
# It will be overwritten by the ALTER SYSTEM command.
primary_conninfo = 'host=node_A'
primary_conninfo = 'host=node_B'
which is expected, and works because the last entry in the file is evaluated, so
on startup, standby "C" follows standby "B".
However, executing "ALTER SYSTEM SET primary_conninfo = 'host=someothernode'"
left
standby "C"'s "postgresql.auto.conf" file looking like this:
# Do not edit this file manually!
# It will be overwritten by the ALTER SYSTEM command.
primary_conninfo = 'host=someothernode'
primary_conninfo = 'host=node_B'
which seems somewhat broken, to say the least.
As-is, the user will either need to repeatedly issue "ALTER SYSTEM RESET
primary_conninfo"
until the duplicates are cleared (which means "ALTER SYSTEM RESET ..." doesn't
work as
advertised, and is not an obvious solution anyway), or ignore the "Do not edit this
file manually!"
warning and remove the offending entry/entries (which, if done safely, should
involve
shutting down the instance first).
Note this issue is not specific to pg_basebackup, primary_conninfo (or any
other settings
formerly in recovery.conf), it has just manifested itself as the built-in
toolset as of now
provides a handy way of getting into this situation without too much effort
(and any
utilities which clone standbys and append the replication configuration to
"postgresql.auto.conf" in lieu of creating "recovery.conf" will be prone to
running into
the same situation).
I had previously always assumed that ALTER SYSTEM would change the *last*
occurrence for
the parameter in "postgresql.auto.conf", in the same way you'd need to be sure
to change
the last occurrence in the normal configuration files, however this actually
not the case -
as per replace_auto_config_value() ( src/backend/utils/misc/guc.c ):
/* Search the list for an existing match (we assume there's only one) */
the *first* match is replaced.
Attached patch attempts to rectify this situation by having
replace_auto_config_value()
deleting any duplicate entries first, before making any changes to the last
entry.
Arguably it might be sufficient (and simpler) to just scan the list for the last
entry, but removing preceding duplicates seems cleaner, as it's pointless
(and a potential source of confusion) keeping entries around which will never
be used.
Also attached is a set of TAP tests to check ALTER SYSTEM works as expected (or
at least as seems correct to me).
Regards
Ian Barwick
--
Ian Barwick https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
new file mode 100644
index 1208eb9..5287004
*** a/src/backend/utils/misc/guc.c
--- b/src/backend/utils/misc/guc.c
*** write_auto_conf_file(int fd, const char
*** 7845,7894
/*
* Update the given list of configuration parameters, adding, replacing
* or deleting the entry for item "name" (delete if "value" == NULL).
*/
static void
replace_auto_config_value(ConfigVariable **head_p, ConfigVariable **tail_p,
const char *name, const char *value)
{
! ConfigVariable *item,
! *prev = NULL;
Re: doc: pg_trgm missing description for GUC "pg_trgm.strict_word_similarity_threshold"
On 6/11/19 2:33 AM, Alexander Korotkov wrote: On Sat, Jun 8, 2019 at 8:17 PM Alexander Korotkov wrote: On Fri, Jun 7, 2019 at 6:02 PM Ian Barwick wrote: On 6/7/19 9:00 PM, Michael Paquier wrote: On Fri, Jun 07, 2019 at 03:44:14PM +0900, Masahiko Sawada wrote: > Or is that not worth bothering except on HEAD? Thoughts? Personally I don't think it's that critical, but not bothered either way. Presumably no-one has complained so far anyway (I only chanced upon the missing GUC description because I was poking about looking for examples of custom GUC handling...) I think it worth maintaining consistent documentation and GUC descriptions in back branches. So, I'm +1 for backpatching. I'm going to commit all 3 patches (documentation, GUC description, documentation indentation) on no objections. Pushed! Thanks! Regards Ian Barwick -- Ian Barwick https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
doc: clarify "pg_signal_backend" default role
Hi Currently the documentation for the default role "pg_signal_backend" states, somewhat ambiguously, "Send signals to other backends (eg: cancel query, terminate)", giving the impression other signals (e.g. SIGHUP) can be sent too, which is currently not the case. Attached patch clarifies this, adds a descriptive paragraph (similar to what the other default roles have) and a link to the "Server Signaling Functions" section. Patch applies cleanly to HEAD and REL_11_STABLE. Regards Ian Barwick -- Ian Barwick https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services diff --git a/doc/src/sgml/user-manag.sgml b/doc/src/sgml/user-manag.sgml new file mode 100644 index 6106244..76b6ddb *** a/doc/src/sgml/user-manag.sgml --- b/doc/src/sgml/user-manag.sgml *** DROP ROLE doomed_role; *** 532,538 pg_signal_backend !Send signals to other backends (eg: cancel query, terminate). pg_read_server_files --- 532,538 pg_signal_backend !Signal another backend to cancel a query or terminate the backend. pg_read_server_files *** DROP ROLE doomed_role; *** 561,566 --- 561,573 + The pg_signal_backend role is intended to allow administrators to enable + trusted, but non-superuser, roles to send signals to other backends. Currently this role + enables sending of signals for canceling a query on another backend or terminating the + backend. A user granted this role cannot however send signals to a backend owned by a superuser. + See also . + + The pg_read_server_files, pg_write_server_files and pg_execute_server_program roles are intended to allow administrators to have trusted, but non-superuser, roles which are able to access files and run programs on the
Re: doc: pg_trgm missing description for GUC "pg_trgm.strict_word_similarity_threshold"
On 6/7/19 9:00 PM, Michael Paquier wrote: On Fri, Jun 07, 2019 at 03:44:14PM +0900, Masahiko Sawada wrote: BTW while looking GUC variables defined in trgm_op.c the operators in each short description seems not correct; there is an extra percent sign. Should we also fix them? Both of you are right here I did notice the double percent signs but my brain skipped over them assuming they were translatable strings, thanks for catching that. and the addition documentation looks fine to me (except the indentation). The indentation in the additional documentation seems fine to me, it's the section for the preceding GUC which is offset one column to the right. Patch attached for that. > The fix for the parameter descriptions can be back-patched safely as they > would reload correctly once the version is updated. Yup, they would appear the first time one of the pg_trgm functions is called in a session after the new object file is installed. > Or is that not worth bothering except on HEAD? Thoughts? Personally I don't think it's that critical, but not bothered either way. Presumably no-one has complained so far anyway (I only chanced upon the missing GUC description because I was poking about looking for examples of custom GUC handling...) Regards Ian Barwick -- Ian Barwick https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services diff --git a/doc/src/sgml/pgtrgm.sgml b/doc/src/sgml/pgtrgm.sgml index 83b0033d7a..e353ecc954 100644 --- a/doc/src/sgml/pgtrgm.sgml +++ b/doc/src/sgml/pgtrgm.sgml @@ -320,23 +320,23 @@ - - - pg_trgm.word_similarity_threshold (real) - - -pg_trgm.word_similarity_threshold configuration parameter - - - - - - Sets the current word similarity threshold that is used by - % and % operators. The threshold - must be between 0 and 1 (default is 0.6). - - - + + + pg_trgm.word_similarity_threshold (real) + + + pg_trgm.word_similarity_threshold configuration parameter + + + + + + Sets the current word similarity threshold that is used by + % and % operators. The threshold + must be between 0 and 1 (default is 0.6). + + +
doc: pg_trgm missing description for GUC "pg_trgm.strict_word_similarity_threshold"
Hi Commit be8a7a68662 added custom GUC "pg_trgm.strict_word_similarity_threshold", but omitted to document this in the section "GUC Parameters"; proposed patch attached. I suggest backpatching to Pg11, where it was introduced. Regards Ian Barwick -- Ian Barwick https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services diff --git a/doc/src/sgml/pgtrgm.sgml b/doc/src/sgml/pgtrgm.sgml index 83b0033d7a..463df46bdd 100644 --- a/doc/src/sgml/pgtrgm.sgml +++ b/doc/src/sgml/pgtrgm.sgml @@ -337,6 +337,20 @@ + + + pg_trgm.strict_word_similarity_threshold (real) + + pg_trgm.strict_word_similarity_threshold configuration parameter + + + + + Sets the current strict word similarity threshold that is used by the % + and % operators. The threshold must be between 0 and 1 (default is 0.5). + + +
Re: PG 12 draft release notes
On 5/22/19 4:26 PM, Michael Paquier wrote: On Wed, May 22, 2019 at 09:19:53AM +0900, Ian Barwick wrote: the last two items are performance improvements not related to authentication; presumably the VACUUM item would be better off in the "Utility Commands" section and the TRUNCATE item in "General Performance"? I agree with removing them from authentication, but these are not performance-related items. Instead I think that "Utility commands" is a place where they can live better. I am wondering if we should insist on the DOS attacks on a server, as non-authorized users are basically able to block any tables, and authorization is only a part of it, one of the worst parts actually... Anyway, I think that "This prevents unauthorized locking delays." does not provide enough details. What about reusing the first paragraph of the commits? Here is an idea: "A caller of TRUNCATE/VACUUM/ANALYZE could previously queue for an access exclusive lock on a relation it may not have permission to truncate/vacuum/analyze, potentially interfering with users authorized to work on it. This could prevent users from accessing some relations they have access to, in some cases preventing authentication if a critical catalog relation was blocked." Ah, if that's the intent behind/use for those changes (I haven't looked at them in any detail, was just scanning the release notes) then it certainly needs some explanation along those lines. Regards Ian Barwick -- Ian Barwick https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: PG 12 draft release notes
On 5/12/19 5:33 AM, Bruce Momjian wrote: I have posted a draft copy of the PG 12 release notes here: http://momjian.us/pgsql_docs/release-12.html They are committed to git. It includes links to the main docs, where appropriate. Our official developer docs will rebuild in a few hours. In section "Authentication": https://www.postgresql.org/docs/devel/release-12.html#id-1.11.6.5.5.3.7 the last two items are performance improvements not related to authentication; presumably the VACUUM item would be better off in the "Utility Commands" section and the TRUNCATE item in "General Performance"? In section "Source code": https://www.postgresql.org/docs/devel/release-12.html#id-1.11.6.5.5.12 the item "Add CREATE ACCESS METHOD command" doesn't seem related to the source code itself, though I'm not sure where it should go. Regards Ian Barwick -- Ian Barwick https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: PG12, PGXS and linking pgfeutils
On 5/15/19 3:38 AM, Tom Lane wrote: I wrote: I think moving fe_utils/logging.[hc] to common/ is definitely the way to get out of this problem. I've pushed that, so Ian's problem should be gone as of HEAD. Thanks, that resolves the issue! Regards Ian Barwick -- Ian Barwick https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
PG12, PGXS and linking pgfeutils
Hi
Commit cc8d4151 [*] introduced a dependency between some functions in
libpgcommon and libpgfeutils, which is not reflected in the linker options
provided when building an external program using PGXS, e.g. attempting to
build the attached (trivial) example results in:
$ PATH=$PG_HEAD:$PATH USE_PGXS=1 make
gcc -std=gnu99 -Wall -Wmissing-prototypes -Wpointer-arith
-Wdeclaration-after-statement -Werror=vla -Wendif-labels
-Wmissing-format-attribute -Wformat-security -fno-strict-aliasing -fwrapv
-fexcess-precision=standard -g -ggdb -Og -g3 -fno-omit-frame-pointer -I. -I./
-I/home/ibarwick/devel/builds/HEAD/include/postgresql/server
-I/home/ibarwick/devel/builds/HEAD/include/postgresql/internal -D_GNU_SOURCE
-c -o pgxs-test.o pgxs-test.c
gcc -std=gnu99 -Wall -Wmissing-prototypes -Wpointer-arith
-Wdeclaration-after-statement -Werror=vla -Wendif-labels
-Wmissing-format-attribute -Wformat-security -fno-strict-aliasing -fwrapv
-fexcess-precision=standard -g -ggdb -Og -g3 -fno-omit-frame-pointer
pgxs-test.o -L/home/ibarwick/devel/builds/HEAD/lib -Wl,--as-needed
-Wl,-rpath,'/home/ibarwick/devel/builds/HEAD/lib',--enable-new-dtags
-L/home/ibarwick/devel/builds/HEAD/lib -lpgcommon -lpgport
-L/home/ibarwick/devel/builds/HEAD/lib -lpq -lpgcommon -lpgport -lpthread -lssl
-lcrypto -lgssapi_krb5 -lz -lreadline -lrt -lcrypt -ldl -lm -o pgxs-test
/home/ibarwick/devel/builds/HEAD/lib/libpgcommon.a(pgfnames.o): In function
`pgfnames':
/home/ibarwick/devel/postgresql/src/common/pgfnames.c:48: undefined
reference to `__pg_log_level'
/home/ibarwick/devel/postgresql/src/common/pgfnames.c:48: undefined
reference to `pg_log_generic'
/home/ibarwick/devel/postgresql/src/common/pgfnames.c:69: undefined
reference to `__pg_log_level'
/home/ibarwick/devel/postgresql/src/common/pgfnames.c:69: undefined
reference to `pg_log_generic'
/home/ibarwick/devel/postgresql/src/common/pgfnames.c:74: undefined
reference to `__pg_log_level'
/home/ibarwick/devel/postgresql/src/common/pgfnames.c:74: undefined
reference to `pg_log_generic'
collect2: error: ld returned 1 exit status
make: *** [pgxs-test] Error 1
which is a regression compared to PG11 and earlier.
Workaround/possible fix is to include "pgfeutils" in the "libpq_pgport"
definition, i.e.:
*** a/src/Makefile.global.in
--- b/src/Makefile.global.in
*** libpq = -L$(libpq_builddir) -lpq
*** 561,567
# on client link lines, since that also appears in $(LIBS).
# libpq_pgport_shlib is the same idea, but for use in client shared
libraries.
ifdef PGXS
! libpq_pgport = -L$(libdir) -lpgcommon -lpgport $(libpq)
libpq_pgport_shlib = -L$(libdir) -lpgcommon_shlib -lpgport_shlib $(libpq)
else
libpq_pgport = -L$(top_builddir)/src/common -lpgcommon
-L$(top_builddir)/src/port -lpgport $(libpq)
--- 561,567
# on client link lines, since that also appears in $(LIBS).
# libpq_pgport_shlib is the same idea, but for use in client shared
libraries.
ifdef PGXS
! libpq_pgport = -L$(libdir) -lpgcommon -lpgport -lpgfeutils $(libpq)
libpq_pgport_shlib = -L$(libdir) -lpgcommon_shlib -lpgport_shlib $(libpq)
else
libpq_pgport = -L$(top_builddir)/src/common -lpgcommon
-L$(top_builddir)/src/port -lpgport $(libpq)
I presume a similar modification may need to be added to the following lines in
that section but haven't had a chance to look in detail yet (and may be barking
up the wrong tree entirely of course).
[*]
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=cc8d41511721d25d557fc02a46c053c0a602fed
Regards
Ian Barwick
--
Ian Barwick https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
PROGRAM = pgxs-test
OBJS = pgxs-test.o
PG_CONFIG = pg_config
PG_LIBS = $(libpq_pgport)
PGXS := $(shell $(PG_CONFIG) --pgxs)
include $(PGXS)
#include "postgres.h"
#include "fmgr.h"
int
main(int argc, char **argv)
{
char **fnames = pgfnames("/tmp");
char *fname = fnames[0];
int total_names = 0;
while (fname)
{
total_names++;
fname = fnames[total_names];
}
printf("total files: %i\n", total_names);
pgfnames_cleanup(fnames);
return 0;
}
Re: Minor comment fix for pg_config_manual.h
On 12/29/18 8:27 AM, Michael Paquier wrote: On Fri, Dec 28, 2018 at 12:37:41AM -0300, Alvaro Herrera wrote: Looks good to me. Thanks for the lookup. I have committed and back-patched to v11 for consistency. Thanks! Regards Ian Barwick -- Ian Barwick http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Small doc tweak for array/string functions
Hi On these pages: - https://www.postgresql.org/docs/current/functions-array.html - https://www.postgresql.org/docs/current/functions-string.html we point out via "See also" the existence of aggregate array and string functions, but I think it would be useful to also mention the existence of string-related array functions and array-related string (regexp) functions respectively. (Background: due to brain fade I was looking on the array functions page for the array-related function whose name was escaping me which does something with regexes to make an array, and was puzzled to find no reference on that page). Regards Ian Barwick -- Ian Barwick http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml new file mode 100644 index 3786099..1684189 *** a/doc/src/sgml/func.sgml --- b/doc/src/sgml/func.sgml *** *** 2363,2369 See also the aggregate function string_agg in !. --- 2363,2371 See also the aggregate function string_agg in !, and the array functions !array_to_string and string_to_array !in . *** NULL baz(3 rows) *** 13163,13169 See also about the aggregate ! function array_agg for use with arrays. --- 13165,13173 See also about the aggregate ! function array_agg for use with arrays, and ! about the regular expression function ! regexp_split_to_array.
Minor comment fix for pg_config_manual.h
Hi
Attached is mainly to fix a comment in $subject which has a typo in the
referenced initdb
option ("--walsegsize", should be "--wal-segsize"), and while I'm there also
adds a
couple of "the" for readability.
Regards
Ian Barwick
--
Ian Barwick http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
diff --git a/src/include/pg_config_manual.h b/src/include/pg_config_manual.h
new file mode 100644
index cc5eedf..cf1cfb4
*** a/src/include/pg_config_manual.h
--- b/src/include/pg_config_manual.h
***
*** 14,21
*/
/*
! * This is default value for wal_segment_size to be used at initdb when run
! * without --walsegsize option. Must be a valid segment size.
*/
#define DEFAULT_XLOG_SEG_SIZE (16*1024*1024)
--- 14,21
*/
/*
! * This is the default value for wal_segment_size to be used at initdb when run
! * without the --wal-segsize option. Must be a valid segment size.
*/
#define DEFAULT_XLOG_SEG_SIZE (16*1024*1024)
Re: Function to promote standby servers
On 11/19/2018 01:22 PM, Michael Paquier wrote: On Fri, Oct 26, 2018 at 01:51:24PM +0900, Michael Paquier wrote: Or we could use "the function returns true immediately after initiating the promotion by sending the promotion signal to the postmaster"? As a native speaker which one feels more natural to you? Sorry for the time it took. After pondering on it, I have committed the improvements from your version. Thanks, looks good (and apologies for the delay in responding from my side). Regards Ian Barwick -- Ian Barwick http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: Function to promote standby servers
Hi On 10/25/2018 09:47 AM, Michael Paquier wrote: And committed. I double-checked the code, and tweaked a bit the tests so as the test using wait_mode = false is removed as it did not seem worth the extra cycles. I also added a check on the return value of pg_promote when using the wait mode. Another thing was that the function needs to be parallel-restricted. Documentation for this [*] says "Returns true if promotion is successful and false otherwise", which is not correct if "wait" is false, as it will always return TRUE. [*] https://www.postgresql.org/docs/devel/static/functions-admin.html#FUNCTIONS-RECOVERY-CONTROL Attached patch contains a suggested rewording to clarify this. Regards Ian Barwick -- Ian Barwick http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services commit 208247998484ee70cff5f16050344d53bcc6bb42 Author: Ian Barwick Date: Fri Oct 26 11:09:21 2018 +0900 Clarify description of pg_promote function Note that it always returns TRUE if the "wait" parameter is set to false. diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml index 96d4541..422af61 100644 --- a/doc/src/sgml/func.sgml +++ b/doc/src/sgml/func.sgml @@ -19241,13 +19241,14 @@ postgres=# SELECT * FROM pg_walfile_name_offset(pg_stop_backup()); boolean -Promotes a physical standby server. Returns true -if promotion is successful and false otherwise. -With wait set to true, the -default, the function waits until promotion is completed or -wait_seconds seconds have passed, otherwise the -function returns immediately after sending the promotion signal to the -postmaster. This function is restricted to superusers by default, but +Promotes a physical standby server. With wait set +to true (the default), the function waits until promotion +is completed or wait_seconds seconds have passed, +and returns true if promotion is successful and +false otherwise. If wait +is set to false, the function returns true +immediately after sending the promotion signal to the postmaster. +This function is restricted to superusers by default, but other users can be granted EXECUTE to run the function.
Re: Function to promote standby servers
Hi On 10/25/2018 09:47 AM, Michael Paquier wrote: And committed. I double-checked the code, and tweaked a bit the tests so as the test using wait_mode = false is removed as it did not seem worth the extra cycles. I also added a check on the return value of pg_promote when using the wait mode. Another thing was that the function needs to be parallel-restricted. Documentation for this [*] says "Returns true if promotion is successful and false otherwise", which is not correct if "wait" is false, as it will always return TRUE. [*] https://www.postgresql.org/docs/devel/static/functions-admin.html#FUNCTIONS-RECOVERY-CONTROL Attached patch contains a suggested rewording to clarify this. Regards Ian Barwick -- Ian Barwick http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services commit 208247998484ee70cff5f16050344d53bcc6bb42 Author: Ian Barwick Date: Fri Oct 26 11:09:21 2018 +0900 Clarify description of pg_promote function Note that it always returns TRUE if the "wait" parameter is set to false. diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml index 96d4541..422af61 100644 --- a/doc/src/sgml/func.sgml +++ b/doc/src/sgml/func.sgml @@ -19241,13 +19241,14 @@ postgres=# SELECT * FROM pg_walfile_name_offset(pg_stop_backup()); boolean -Promotes a physical standby server. Returns true -if promotion is successful and false otherwise. -With wait set to true, the -default, the function waits until promotion is completed or -wait_seconds seconds have passed, otherwise the -function returns immediately after sending the promotion signal to the -postmaster. This function is restricted to superusers by default, but +Promotes a physical standby server. With wait set +to true (the default), the function waits until promotion +is completed or wait_seconds seconds have passed, +and returns true if promotion is successful and +false otherwise. If wait +is set to false, the function returns true +immediately after sending the promotion signal to the postmaster. +This function is restricted to superusers by default, but other users can be granted EXECUTE to run the function.
Re: Add %r substitution for psql prompts to show recovery status
On 01/23/2018 03:19 AM, Peter Eisentraut wrote: On 1/9/18 15:36, Peter Eisentraut wrote: On 12/7/17 19:54, Tatsuo Ishii wrote: On Wed, Dec 6, 2017 at 9:19 PM, Ian Barwick <ian.barw...@2ndquadrant.com> wrote: Note this substitution sends a "pg_is_in_recovery()" query to the server each time it's encountered; unless there's something I'm overlooking I think that's the only reliable way to determine current recovery status. That seems kinda painful. And what happens in an aborted transaction? Yeah. I think we need some from help backend for this. For example, a parameter status message can be used here. If PostgreSQL moves to the recovery state or vice versa, PostgreSQL sends the parameter status message to frontend. I agree a backend status message is the right way to do this. We could perhaps report transaction_read_only, if we don't want to add a new one. I'm setting this CF item to returned with feedback, since it would apparently be a much bigger change than then initial patch. Yup, agree :) Thanks for the feedback! Regards Ian Barwick -- Ian Barwick http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
