Re: Remove libpq.rc, use win32ver.rc for libpq
On Fri, Dec 27, 2019 at 05:25:58PM +0100, Peter Eisentraut wrote: > I was wondering why we have a separate libpq.rc for libpq and use > win32ver.rc for all other components. I suspect this is also a leftover > from the now-removed client-only Windows build. With a bit of tweaking we > can use win32ver.rc for libpq as well and remove a bit of duplicative code. > > I have tested this patch with MSVC and MinGW. The patch does not apply anymore because of two conflicts with the copyright dates, could you rebase it? Reading through it, the change looks sensible. However I have not looked at it yet in details. - FILEFLAGSMASK 0x17L + FILEFLAGSMASK VS_FFI_FILEFLAGSMASK Are you sure with the mapping here? I would have thought that VS_FF_DEBUG is not necessary when using release-quality builds, which is something that can be configured with build.pl, and that it would be better to not enforce VS_FF_PRERELEASE all the time. -- Michael signature.asc Description: PGP signature
pg_basebackup fails on databases with high OIDs
This is a new bug in PG12. When you have a database with an OID above
INT32_MAX (signed), then pg_basebackup fails thus:
pg_basebackup: error: could not get write-ahead log end position from
server: ERROR: value "30" is out of range for type integer
The cause appears to be commit 6b9e875f7286d8535bff7955e5aa3602e188e436.
A possible fix is attached. An alternative to using
OidInputFunctionCall() would be exporting something like oidin_subr().
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From 19ee6b09568b4247c33c2920277dde2fbd3f0ac4 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Sun, 29 Dec 2019 20:15:50 +0100
Subject: [PATCH] Fix base backup with database OIDs larger than INT32_MAX
---
src/backend/replication/basebackup.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/backend/replication/basebackup.c
b/src/backend/replication/basebackup.c
index a73893237a..0e3e0c7a38 100644
--- a/src/backend/replication/basebackup.c
+++ b/src/backend/replication/basebackup.c
@@ -38,6 +38,7 @@
#include "storage/ipc.h"
#include "storage/reinit.h"
#include "utils/builtins.h"
+#include "utils/fmgroids.h"
#include "utils/ps_status.h"
#include "utils/relcache.h"
#include "utils/timestamp.h"
@@ -1316,7 +1317,7 @@ sendDir(const char *path, int basepathlen, bool sizeonly,
List *tablespaces,
if (!sizeonly)
sent = sendFile(pathbuf, pathbuf + basepathlen
+ 1, &statbuf,
- true, isDbDir ?
pg_atoi(lastDir + 1, sizeof(Oid), 0) : InvalidOid);
+ true, isDbDir ?
DatumGetObjectId(OidInputFunctionCall(F_OIDIN, unconstify(char *, lastDir + 1),
0, -1)) : InvalidOid);
if (sent || sizeonly)
{
--
2.24.1
Re: pgbench - use pg logging capabilities
On Fri, Jan 03, 2020 at 01:01:18PM +0100, Fabien COELHO wrote:
> Without looking at the context I thought that argv[0] was the program name,
> which is not the case here. I put it back everywhere, including the DEBUG
> message.
The variable names in Command are confusing IMO...
> Ok. I homogeneised another similar message.
>
> Patch v3 attached hopefully fixes all of the above.
+ pg_log_error("gaussian parameter must be at least "
+ "%f (not %f)", MIN_GAUSSIAN_PARAM, param);
I would keep all the error message strings to be on the same line.
That makes grepping for them easier on the same, and that's the usual
convention even if these are larger than 72-80 characters.
#ifdef DEBUG
- printf("shell parameter name: \"%s\", value: \"%s\"\n", argv[1], res);
+ pg_log_debug("%s: shell parameter name: \"%s\", value: \"%s\"", argv[0],
argv[1], res);
#endif
Worth removing this ifdef?
- fprintf(stderr, "%s", PQerrorMessage(con));
+ pg_log_fatal("unexpected copy in result");
+ pg_log_error("%s", PQerrorMessage(con));
exit(1);
[...]
- fprintf(stderr, "%s", PQerrorMessage(con));
+ pg_log_fatal("cannot count number of branches");
+ pg_log_error("%s", PQerrorMessage(con));
These are inconsistent with the rest, why not combining both?
I think that I would just remove the "debug" variable defined in
pgbench.c all together, and switch the messages for the duration and
the one in executeMetaCommand to use info-level logging..
--
Michael
signature.asc
Description: PGP signature
Re: A rather hackish POC for alternative implementation of WITH TIES
On Fri, Nov 29, 2019 at 8:40 AM Andrew Gierth wrote: > This patch is a rather hacky implementation of the basic idea for > implementing FETCH ... WITH TIES, and potentially also PERCENT, by using > a window function expression to compute a stopping point. > > Large chunks of this (the parser/ruleutils changes, docs, tests) are > taken from Surafel Temesgen's patch. The difference is that the executor > change in my version is minimal: Limit allows a boolean column in the > input to signal the point at which to stop. The planner inserts a > WindowAgg node to compute the necessary condition using the rank() > function. > > The way this is done in the planner isn't (IMO) the best and should > probably be improved; in particular it currently misses some possible > optimizations (most notably constant-folding of the offset+limit > subexpression). I also haven't tested it properly to see whether I broke > anything, though it does pass regression. > > > Unlike most other executor node limit node has implementation for handling backward scan that support cursor operation but your approach didn't do this inherently because it outsource limitNode functionality to window function and window function didn't do this eg. postgres=# begin; BEGIN postgres=# declare c cursor for select i from generate_series(1,100) s(i) order by i fetch first 2 rows with ties; DECLARE CURSOR postgres=# fetch all in c; i --- 1 2 (2 rows) postgres=# fetch backward all in c; ERROR: cursor can only scan forward HINT: Declare it with SCROLL option to enable backward scan. Even with SCROLL option it is not working as limitNode does. It store the result and return in backward scan that use more space than current limit and limit with ties implementation. If am not mistaken the patch also reevaluate limit every time returning row beside its not good for performance its will return incorrect result with limit involving volatile function regards Surafel
Re: pg_basebackup fails on databases with high OIDs
On Mon, Jan 06, 2020 at 09:07:26AM +0100, Peter Eisentraut wrote: > This is a new bug in PG12. When you have a database with an OID above > INT32_MAX (signed), then pg_basebackup fails thus: Yep. Introduced by 6b9e875. > pg_basebackup: error: could not get write-ahead log end position from > server: ERROR: value "30" is out of range for type integer > > The cause appears to be commit 6b9e875f7286d8535bff7955e5aa3602e188e436. > > A possible fix is attached. An alternative to using OidInputFunctionCall() > would be exporting something like oidin_subr(). I think that you would save yourself from a lot of trouble if you do the latter with a subroutine. Not quite like that based on the process context where the call is done, but remember 21f428eb.. -- Michael signature.asc Description: PGP signature
Re: pg_basebackup fails on databases with high OIDs
On Mon, Jan 6, 2020 at 9:21 AM Michael Paquier wrote: > > On Mon, Jan 06, 2020 at 09:07:26AM +0100, Peter Eisentraut wrote: > > This is a new bug in PG12. When you have a database with an OID above > > INT32_MAX (signed), then pg_basebackup fails thus: > > Yep. Introduced by 6b9e875. Indeed. > > pg_basebackup: error: could not get write-ahead log end position from > > server: ERROR: value "30" is out of range for type integer > > > > The cause appears to be commit 6b9e875f7286d8535bff7955e5aa3602e188e436. > > > > A possible fix is attached. An alternative to using OidInputFunctionCall() > > would be exporting something like oidin_subr(). > > I think that you would save yourself from a lot of trouble if you do > the latter with a subroutine. Not quite like that based on the > process context where the call is done, but remember 21f428eb.. +0.5 to avoid calling OidInputFunctionCall()
Re: adding partitioned tables to publications
On Mon, Dec 16, 2019 at 2:50 PM Amit Langote wrote:
>
> Thanks for checking.
>
> On Thu, Dec 12, 2019 at 12:48 AM Peter Eisentraut
> wrote:
> > On 2019-12-06 08:48, Amit Langote wrote:
> > > 0001: Adding a partitioned table to a publication implicitly adds all
> > > its partitions. The receiving side must have tables matching the
> > > published partitions, which is typically the case, because the same
> > > partition tree is defined on both nodes.
> >
> > This looks pretty good to me now. But you need to make all the changed
> > queries version-aware so that you can still replicate from and to older
> > versions. (For example, pg_partition_tree is not very old.)
>
> True, fixed that.
>
> > This part looks a bit fishy:
> >
> > + /*
> > +* If either table is partitioned, skip copying. Individual
> > partitions
> > +* will be copied instead.
> > +*/
> > + if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE ||
> > + remote_relkind == RELKIND_PARTITIONED_TABLE)
> > + {
> > + logicalrep_rel_close(relmapentry, NoLock);
> > + return;
> > + }
> >
> > I don't think you want to filter out a partitioned table on the local
> > side, since (a) COPY can handle that, and (b) it's (as of this patch) an
> > error to have a partitioned table in the subscription table set.
>
> Yeah, (b) is true, so copy_table() should only ever see regular tables
> with only patch 0001 applied.
>
> > I'm not a fan of the new ValidateSubscriptionRel() function. It's too
> > obscure, especially the return value. Doesn't seem worth it.
>
> It went through many variants since I first introduced it, but yeah I
> agree we don't need it if only because of the weird interface.
>
> It occurred to me that, *as of 0001*, we should indeed disallow
> replicating from a regular table on publisher node into a partitioned
> table of the same name on subscriber node (as the earlier patches
> did), because 0001 doesn't implement tuple routing support that would
> be needed to apply such changes.
>
> Attached updated patches.
>
I am planning to review this patch. Currently, it is not applying on
the head so can you rebase it?
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
Re: PATCH: logical_work_mem and logical streaming of large in-progress transactions
On Mon, Jan 6, 2020 at 9:21 AM Dilip Kumar wrote:
>
> On Sat, Jan 4, 2020 at 4:07 PM Amit Kapila wrote:
> >
> >
> > It is better to merge it with the main patch for
> > "Implement-streaming-mode-in-ReorderBuffer", otherwise, it is a bit
> > difficult to review.
> Actually, we can merge 0008, 0009, 0012, 0018 to the main patch
> (0007). Basically, if we merge all of them then we don't need to deal
> with the conflict. I think Tomas has kept them separate so that we
> can review the solution for the schema sent. And, I kept 0018 as a
> separate patch to avoid conflict and rebasing in 0008, 0009 and 0012.
> In the next patch set, I will merge all of them to 0007.
>
Okay, I think we can merge those patches.
> >
> > + /*
> > + * We don't expect direct calls to heap_getnext with valid
> > + * CheckXidAlive for regular tables. Track that below.
> > + */
> > + if (unlikely(TransactionIdIsValid(CheckXidAlive) &&
> > + !(IsCatalogRelation(scan->rs_base.rs_rd) ||
> > + RelationIsUsedAsCatalogTable(scan->rs_base.rs_rd
> > + elog(ERROR, "improper heap_getnext call");
> >
> > Earlier, I thought we don't need to check if it is a regular table in
> > this check, but it is required because output plugins can try to do
> > that
> I did not understand that, can you give some example?
>
I think it can lead to the same problem of concurrent aborts as for
catalog scans.
> >
> > > > > 2. The commit message of this patch refers to Prepared transactions.
> > > > > I think that needs to be changed.
> > > > >
> > > > > 0006-Implement-streaming-mode-in-ReorderBuffer
> > > > > -
> >
> > Few comments on v4-0018-Review-comment-fix-and-refactoring:
> > 1.
> > + if (streaming)
> > + {
> > + /*
> > + * Set the last last of the stream as the final lsn before calling
> > + * stream stop.
> > + */
> > + txn->final_lsn = prev_lsn;
> > + rb->stream_stop(rb, txn);
> > + }
> >
> > Shouldn't we try to final_lsn as is done by Vignesh's patch [2]?
> Isn't it the same, there we are doing while serializing and here we
> are doing while streaming? Basically, the last LSN we streamed. Am I
> missing something?
>
No, I think you are right.
Few more comments:
v4-0007-Implement-streaming-mode-in-ReorderBuffer
1.
+ReorderBufferStreamTXN(ReorderBuffer *rb, ReorderBufferTXN *txn)
{
..
+ /*
+ * TOCHECK: We have to rebuild historic snapshot to be sure it includes all
+ * information about
subtransactions, which could arrive after streaming start.
+ */
+ if (!txn->is_schema_sent)
+ snapshot_now
= ReorderBufferCopySnap(rb, txn->base_snapshot,
+ txn,
command_id);
..
}
Why are we using base snapshot here instead of the snapshot we saved
the first time streaming has happened? And as mentioned in comments,
won't we need to consider the snapshots for subtransactions that
arrived after the last time we have streamed the changes?
2.
+ /* remember the command ID and snapshot for the streaming run */
+ txn->command_id = command_id;
+ txn-
>snapshot_now = ReorderBufferCopySnap(rb, snapshot_now,
+
txn, command_id);
I don't see where the txn->snapshot_now is getting freed. The
base_snapshot is freed in ReorderBufferCleanupTXN, but I don't see
this getting freed.
3.
+static void
+ReorderBufferStreamTXN(ReorderBuffer *rb, ReorderBufferTXN *txn)
{
..
+ /*
+ * If this is a subxact, we need to stream the top-level transaction
+ * instead.
+ */
+ if (txn->toptxn)
+ {
+
ReorderBufferStreamTXN(rb, txn->toptxn);
+ return;
+ }
Is it ever possible that we reach here for subtransaction, if not,
then it should be Assert rather than if condition?
4. In ReorderBufferStreamTXN(), don't we need to set some of the txn
fields like origin_id, origin_lsn as we do in ReorderBufferCommit()
especially to cover the case when it gets called due to memory
overflow (aka via ReorderBufferCheckMemoryLimit).
v4-0017-Extend-handling-of-concurrent-aborts-for-streamin
1.
@@ -3712,7 +3727,22 @@ ReorderBufferStreamTXN(ReorderBuffer *rb,
ReorderBufferTXN *txn)
if (using_subtxn)
RollbackAndReleaseCurrentSubTransaction();
- PG_RE_THROW();
+ /* re-throw only if it's not an abort */
+ if (errdata-
>sqlerrcode != ERRCODE_TRANSACTION_ROLLBACK)
+ {
+ MemoryContextSwitchTo(ecxt);
+ PG_RE_THROW();
+
}
+ else
+ {
+ /* remember the command ID and snapshot for the streaming run */
+ txn-
>command_id = command_id;
+ txn->snapshot_now = ReorderBufferCopySnap(rb, snapshot_now,
+
txn, command_id);
+ rb->stream_stop(rb, txn);
+
+
FlushErrorState();
+ }
Can you update comments either in the above code block or some other
place to explain what is the concurrent abort problem and how we dealt
with it? Also, please explain how the above error handling is
sufficient to address all the various scenarios (sub-transaction got
aborted when we have already sent some changes, or when we have not
sent any changes yet).
v4-0006-Gracefully-handle-concurrent-aborts-of-uncommitte
1.
+ /*
+ * If CheckXi
Re: PATCH: logical_work_mem and logical streaming of large in-progress transactions
On Mon, Jan 6, 2020 at 2:11 PM Amit Kapila wrote:
>
> On Mon, Jan 6, 2020 at 9:21 AM Dilip Kumar wrote:
> >
> > On Sat, Jan 4, 2020 at 4:07 PM Amit Kapila wrote:
> > >
> > >
> > > It is better to merge it with the main patch for
> > > "Implement-streaming-mode-in-ReorderBuffer", otherwise, it is a bit
> > > difficult to review.
> > Actually, we can merge 0008, 0009, 0012, 0018 to the main patch
> > (0007). Basically, if we merge all of them then we don't need to deal
> > with the conflict. I think Tomas has kept them separate so that we
> > can review the solution for the schema sent. And, I kept 0018 as a
> > separate patch to avoid conflict and rebasing in 0008, 0009 and 0012.
> > In the next patch set, I will merge all of them to 0007.
> >
>
> Okay, I think we can merge those patches.
ok
>
> > >
> > > + /*
> > > + * We don't expect direct calls to heap_getnext with valid
> > > + * CheckXidAlive for regular tables. Track that below.
> > > + */
> > > + if (unlikely(TransactionIdIsValid(CheckXidAlive) &&
> > > + !(IsCatalogRelation(scan->rs_base.rs_rd) ||
> > > + RelationIsUsedAsCatalogTable(scan->rs_base.rs_rd
> > > + elog(ERROR, "improper heap_getnext call");
> > >
> > > Earlier, I thought we don't need to check if it is a regular table in
> > > this check, but it is required because output plugins can try to do
> > > that
> > I did not understand that, can you give some example?
> >
>
> I think it can lead to the same problem of concurrent aborts as for
> catalog scans.
Yeah, got it.
>
> > >
> > > > > > 2. The commit message of this patch refers to Prepared transactions.
> > > > > > I think that needs to be changed.
> > > > > >
> > > > > > 0006-Implement-streaming-mode-in-ReorderBuffer
> > > > > > -
> > >
> > > Few comments on v4-0018-Review-comment-fix-and-refactoring:
> > > 1.
> > > + if (streaming)
> > > + {
> > > + /*
> > > + * Set the last last of the stream as the final lsn before calling
> > > + * stream stop.
> > > + */
> > > + txn->final_lsn = prev_lsn;
> > > + rb->stream_stop(rb, txn);
> > > + }
> > >
> > > Shouldn't we try to final_lsn as is done by Vignesh's patch [2]?
> > Isn't it the same, there we are doing while serializing and here we
> > are doing while streaming? Basically, the last LSN we streamed. Am I
> > missing something?
> >
>
> No, I think you are right.
>
> Few more comments:
>
> v4-0007-Implement-streaming-mode-in-ReorderBuffer
> 1.
> +ReorderBufferStreamTXN(ReorderBuffer *rb, ReorderBufferTXN *txn)
> {
> ..
> + /*
> + * TOCHECK: We have to rebuild historic snapshot to be sure it includes all
> + * information about
> subtransactions, which could arrive after streaming start.
> + */
> + if (!txn->is_schema_sent)
> + snapshot_now
> = ReorderBufferCopySnap(rb, txn->base_snapshot,
> + txn,
> command_id);
> ..
> }
>
> Why are we using base snapshot here instead of the snapshot we saved
> the first time streaming has happened? And as mentioned in comments,
> won't we need to consider the snapshots for subtransactions that
> arrived after the last time we have streamed the changes?
>
> 2.
> + /* remember the command ID and snapshot for the streaming run */
> + txn->command_id = command_id;
> + txn-
> >snapshot_now = ReorderBufferCopySnap(rb, snapshot_now,
> +
> txn, command_id);
>
> I don't see where the txn->snapshot_now is getting freed. The
> base_snapshot is freed in ReorderBufferCleanupTXN, but I don't see
> this getting freed.
Ok, I will check that and fix.
>
> 3.
> +static void
> +ReorderBufferStreamTXN(ReorderBuffer *rb, ReorderBufferTXN *txn)
> {
> ..
> + /*
> + * If this is a subxact, we need to stream the top-level transaction
> + * instead.
> + */
> + if (txn->toptxn)
> + {
> +
> ReorderBufferStreamTXN(rb, txn->toptxn);
> + return;
> + }
>
> Is it ever possible that we reach here for subtransaction, if not,
> then it should be Assert rather than if condition?
ReorderBufferCheckMemoryLimit, can call it either for the
subtransaction or for the main transaction, depends upon in which
ReorderBufferTXN you are adding the current change.
I will analyze your other comments and fix them in the next version.
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
Re: logical replication does not fire per-column triggers
On 2019-12-16 14:37, Peter Eisentraut wrote: New patch attached. I have committed this and backpatched it to PG10. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: pgsql: Add basic TAP tests for psql's tab-completion logic.
Re: Tom Lane 2020-01-05 <25771.1578249...@sss.pgh.pa.us> > The current state of play on this is that I committed a hacky workaround > [1], but there is now a fix for it in libedit upstream [2][3]. I gather > from looking at Debian's package page that the fix could be expected to > propagate to Debian unstable within a few weeks, at which point I'd like > to revert the hack. The libedit bug's only been there a few months > (it was evidently introduced on 2019-03-31) so we can hope that it hasn't > propagated into any long-term-support distros. [...] I lost track of what bug is supposed to be where, so here's a summary of the state at apt.postgresql.org: PG13 head work on Debian unstable, buster, stretch. Does not work on Ubuntu bionic, xenial. (Others not tested.) Ubuntu xenial: 07:24:42 # Failed test 'complete SEL to SELECT' 07:24:42 # at t/010_tab_completion.pl line 98. 07:24:42 # Actual output was "SEL\tpostgres=# SEL\a" 07:24:42 # Did not match "(?^:SELECT )" 07:24:48 07:24:48 # Failed test 'complete sel to select' 07:24:48 # at t/010_tab_completion.pl line 103. 07:24:48 # Actual output was "sel\b\b\bSELECT " 07:24:48 # Did not match "(?^:select )" 07:24:54 07:24:54 # Failed test 'complete t to tab1' 07:24:54 # at t/010_tab_completion.pl line 106. 07:24:54 # Actual output was "* from t " 07:24:54 # Did not match "(?^:\* from tab1 )" 07:25:00 07:25:00 # Failed test 'complete my to mytab when there are multiple choices' 07:25:00 # at t/010_tab_completion.pl line 112. 07:25:00 # Actual output was "select * from my " 07:25:00 # Did not match "(?^:select \* from my\a?tab)" 07:25:06 07:25:06 # Failed test 'offer multiple table choices' 07:25:06 # at t/010_tab_completion.pl line 118. 07:25:06 # Actual output was "\r\n\r\n\r\r\npostgres=# select * from my \r\n\r\n\r\r\npostgres=# select * from my " 07:25:06 # Did not match "(?^:mytab123 +mytab246)" 07:25:12 07:25:12 # Failed test 'finish completion of one of multiple table choices' 07:25:12 # at t/010_tab_completion.pl line 123. 07:25:12 # Actual output was "2 " 07:25:12 # Did not match "(?^:246 )" 07:25:18 07:25:18 # Failed test 'complete \DRD to \drds' 07:25:18 # at t/010_tab_completion.pl line 131. 07:25:18 # Actual output was "\\DRD\b\b\b\bselect " 07:25:18 # Did not match "(?^:drds )" 07:25:18 # Looks like you failed 7 tests of 12. 07:25:18 t/010_tab_completion.pl .. 07:25:18 Dubious, test returned 7 (wstat 1792, 0x700) 07:25:18 Failed 7/12 subtests Ubuntu bionic fails elsewhere: 07:19:51 t/001_stream_rep.pl .. ok 07:19:53 t/002_archiving.pl ... ok 07:19:59 t/003_recovery_targets.pl ok 07:20:01 t/004_timeline_switch.pl . ok 07:20:08 t/005_replay_delay.pl ok 07:20:10 Bailout called. Further testing stopped: system pg_ctl failed 07:20:10 FAILED--Further testing stopped: system pg_ctl failed 07:20:10 2020-01-06 06:19:41.285 UTC [26415] LOG: received fast shutdown request 07:20:10 2020-01-06 06:19:41.285 UTC [26415] LOG: aborting any active transactions 07:20:10 2020-01-06 06:19:41.287 UTC [26415] LOG: background worker "logical replication launcher" (PID 26424) exited with exit code 1 07:20:10 2020-01-06 06:19:41.287 UTC [26419] LOG: shutting down 07:20:10 2020-01-06 06:19:41.287 UTC [26419] LOG: checkpoint starting: shutdown immediate (It didn't get to the 010_tab_completion.pl test.) Libedit versions are: Debian: libedit2 | 3.1-20140620-2 | oldoldstable | amd64, armel, armhf, i386 (jessie) libedit2 | 3.1-20160903-3 | oldstable| amd64, arm64, armel, armhf, i386, mips, mips64el, m (stretch) libedit2 | 3.1-20181209-1 | stable | amd64, arm64, armel, armhf, i386, mips, mips64el, m (buster) libedit2 | 3.1-20191211-1 | testing | amd64, arm64, armel, armhf, i386, mips64el, mipsel, (bullseye) libedit2 | 3.1-20191231-1 | unstable | amd64, arm64, armel, armhf, i386, mips64el, mipsel, Ubuntu: libedit2 | 2.11-20080614-3ubuntu2 | precise| amd64, armel, armhf, i386, powerpc libedit2 | 3.1-20130712-2 | trusty | amd64, arm64, armhf, i386, powerpc, ppc64e libedit2 | 3.1-20150325-1ubuntu2 | xenial | amd64, arm64, armhf, i386, powerpc, ppc64e libedit2 | 3.1-20170329-1 | bionic | amd64, arm64, armhf, i386, ppc64el, s390x libedit2 | 3.1-20181209-1 | disco | amd64, arm64, armhf, i386, ppc64el, s390x libedit2 | 3.1-20190324-1 | eoan | amd64, arm64, armhf, i386, ppc64el, s390x libedit2 | 3.1-20191211-1 | focal | amd64, arm64, armhf, i386, ppc64el, s390x libedit2 | 3.1-20191231-1 | focal-proposed | amd64, arm64, armhf, i386, ppc64el, s390x Christoph
Re: [Proposal] Global temporary tables
On Mon, Jan 06, 2020 at 01:04:15PM +0800, 曾文旌(义从) wrote: In the previous communication 1 we agreed on the general direction 1.1 gtt use local (private) buffer 1.2 no replica access in first version OK, good. 2 We feel that gtt needs to maintain statistics, but there is no agreement on what it will be done. I certainly agree GTT needs to maintain statistics, otherwise it'll lead to poor query plans. AFAIK the current patch stores the info in a hash table in a backend private memory, and I don't see how else to do that (e.g. storing it in a catalog would cause catalog bloat). FWIW this is a reasons why I think just using shared buffers (instead of local ones) is not sufficient to support parallel queriesl as proposed by Alexander. The workers would not know the stats, breaking planning of queries in PARALLEL SAFE plpgsql functions etc. 3 Still no one commented on GTT's transaction information processing, they include 3.1 Should gtt's frozenxid need to be care? 3.2 gtt’s clog clean 3.3 How to deal with "too old" gtt data No idea what to do about this. I suggest we discuss further, reach an agreement, and merge the two patches to one. OK, cool. Thanks for the clarification. regards -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: PATCH: logical_work_mem and logical streaming of large in-progress transactions
On Mon, Jan 6, 2020 at 3:56 PM Dilip Kumar wrote:
>
> On Mon, Jan 6, 2020 at 2:11 PM Amit Kapila wrote:
> >
> > 3.
> > +static void
> > +ReorderBufferStreamTXN(ReorderBuffer *rb, ReorderBufferTXN *txn)
> > {
> > ..
> > + /*
> > + * If this is a subxact, we need to stream the top-level transaction
> > + * instead.
> > + */
> > + if (txn->toptxn)
> > + {
> > +
> > ReorderBufferStreamTXN(rb, txn->toptxn);
> > + return;
> > + }
> >
> > Is it ever possible that we reach here for subtransaction, if not,
> > then it should be Assert rather than if condition?
>
> ReorderBufferCheckMemoryLimit, can call it either for the
> subtransaction or for the main transaction, depends upon in which
> ReorderBufferTXN you are adding the current change.
>
That function has code like below:
ReorderBufferCheckMemoryLimit()
{
..
if (ReorderBufferCanStream(rb))
{
/*
* Pick the largest toplevel transaction and evict it from memory by
* streaming the already decoded part.
*/
txn = ReorderBufferLargestTopTXN(rb);
/* we know there has to be one, because the size is not zero */
Assert(txn && !txn->toptxn);
..
ReorderBufferStreamTXN(rb, txn);
..
}
How can it ReorderBufferTXN pass for subtransaction?
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
Re: PATCH: logical_work_mem and logical streaming of large in-progress transactions
On Mon, Jan 6, 2020 at 4:36 PM Amit Kapila wrote:
>
> On Mon, Jan 6, 2020 at 3:56 PM Dilip Kumar wrote:
> >
> > On Mon, Jan 6, 2020 at 2:11 PM Amit Kapila wrote:
> > >
> > > 3.
> > > +static void
> > > +ReorderBufferStreamTXN(ReorderBuffer *rb, ReorderBufferTXN *txn)
> > > {
> > > ..
> > > + /*
> > > + * If this is a subxact, we need to stream the top-level transaction
> > > + * instead.
> > > + */
> > > + if (txn->toptxn)
> > > + {
> > > +
> > > ReorderBufferStreamTXN(rb, txn->toptxn);
> > > + return;
> > > + }
> > >
> > > Is it ever possible that we reach here for subtransaction, if not,
> > > then it should be Assert rather than if condition?
> >
> > ReorderBufferCheckMemoryLimit, can call it either for the
> > subtransaction or for the main transaction, depends upon in which
> > ReorderBufferTXN you are adding the current change.
> >
>
> That function has code like below:
>
> ReorderBufferCheckMemoryLimit()
> {
> ..
> if (ReorderBufferCanStream(rb))
> {
> /*
> * Pick the largest toplevel transaction and evict it from memory by
> * streaming the already decoded part.
> */
> txn = ReorderBufferLargestTopTXN(rb);
> /* we know there has to be one, because the size is not zero */
> Assert(txn && !txn->toptxn);
> ..
> ReorderBufferStreamTXN(rb, txn);
> ..
> }
>
> How can it ReorderBufferTXN pass for subtransaction?
>
Hmm, I missed it. You are right, will fix it.
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
Re: adding partitioned tables to publications
Hi Amit,
I went through this patch set once again today and here are my two cents.
On Mon, 16 Dec 2019 at 10:19, Amit Langote wrote:
>
> Attached updated patches.
- differently partitioned setup. Attempts to replicate tables other than
- base tables will result in an error.
+ Replication is only supported by regular and partitioned tables, although
+ the table kind must match between the two servers, that is, one cannot
I find the phrase 'table kind' a bit odd, how about something like
type of the table.
/* Only plain tables can be aded to publications. */
- if (tbinfo->relkind != RELKIND_RELATION)
+ /* Only plain and partitioned tables can be added to publications. */
IMHO using regular instead of plain would be more consistent.
+ /*
+ * Find a partition for the tuple contained in remoteslot.
+ *
+ * For insert, remoteslot is tuple to insert. For update and delete, it
+ * is the tuple to be replaced and deleted, repectively.
+ */
Misspelled 'respectively'.
+static void
+apply_handle_tuple_routing(ResultRelInfo *relinfo,
+LogicalRepRelMapEntry *relmapentry,
+EState *estate, CmdType operation,
+TupleTableSlot *remoteslot,
+LogicalRepTupleData *newtup)
+{
+ Relation rel = relinfo->ri_RelationDesc;
+ ModifyTableState *mtstate = NULL;
+ PartitionTupleRouting *proute = NULL;
+ ResultRelInfo *partrelinfo,
+*partrelinfo1;
IMHO, partrelinfo1 can be better named to improve readability.
Otherwise, as Dilip already mentioned, there is a rebase required
particularly for 0003 and 0004.
--
Regards,
Rafia Sabih
Re: Greatest Common Divisor
On Sat, Jan 4, 2020 at 4:21 PM Fabien COELHO wrote: > In GCD implementations, for instance: > > if (arg1 == PG_INT32_MIN) > if (arg2 == 0 || arg2 == PG_INT32_MIN) > > And possibly a "likely" on the while. I don't think decoration the code with likely() and unlikely() all over the place is a very good idea. Odds are good that we'll end up with a bunch that are actually non-optimal, and nobody will ever figure it out because it's hard to figure out. I have a hard time believing that we're going to be much worse off if we just write the code normally. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: [Proposal] Global temporary tables
On Mon, 6 Jan 2020 at 11:01, Tomas Vondra wrote: > > On Mon, Jan 06, 2020 at 01:04:15PM +0800, 曾文旌(义从) wrote: > > >2 We feel that gtt needs to maintain statistics, but there is no > >agreement on what it will be done. > > > > I certainly agree GTT needs to maintain statistics, otherwise it'll lead > to poor query plans. +1 > AFAIK the current patch stores the info in a hash > table in a backend private memory, and I don't see how else to do that > (e.g. storing it in a catalog would cause catalog bloat). > It sounds like it needs a pair of system GTTs to hold the table and column statistics for other GTTs. One would probably have the same columns as pg_statistic, and the other just the relevant columns from pg_class. I can see it being useful for the user to be able to see these stats, so perhaps they could be UNIONed into the existing stats view. Regards, Dean
ALTER TABLE ... SET STORAGE does not propagate to indexes
ALTER TABLE ... SET STORAGE does not propagate to indexes, even though
indexes created afterwards get the new storage setting. So depending on
the order of commands, you can get inconsistent storage settings between
indexes and tables. For example:
create table foo1 (a text);
alter table foo1 alter column a set storage external;
create index foo1i on foo1(a);
insert into foo1 values(repeat('a', 1));
ERROR: index row requires 10016 bytes, maximum size is 8191
(Storage "external" disables compression.)
but
create table foo1 (a text);
create index foo1i on foo1(a);
alter table foo1 alter column a set storage external;
insert into foo1 values(repeat('a', 1));
-- no error
Also, this second state cannot be reproduced by pg_dump, so a possible
effect is that such a database would fail to restore.
Attached is a patch that attempts to fix this by propagating the storage
change to existing indexes. This triggers a few regression test
failures (going from no error to error), which I attempted to fix up,
but I haven't analyzed what the tests were trying to do, so it might
need another look.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From 85f19d31a2def2e2b98d669daa4bd4a3a2c2c09f Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Mon, 6 Jan 2020 11:54:03 +0100
Subject: [PATCH] Propagate ALTER TABLE ... SET STORAGE to indexes
---
contrib/test_decoding/expected/toast.out | 4 +--
contrib/test_decoding/sql/toast.sql | 4 +--
src/backend/commands/tablecmds.c | 33
src/test/regress/expected/vacuum.out | 4 +--
src/test/regress/sql/vacuum.sql | 4 +--
5 files changed, 41 insertions(+), 8 deletions(-)
diff --git a/contrib/test_decoding/expected/toast.out
b/contrib/test_decoding/expected/toast.out
index 91a9a1e86d..2baa06f304 100644
--- a/contrib/test_decoding/expected/toast.out
+++ b/contrib/test_decoding/expected/toast.out
@@ -305,8 +305,8 @@ ALTER TABLE toasted_several REPLICA IDENTITY FULL;
ALTER TABLE toasted_several ALTER COLUMN toasted_key SET STORAGE EXTERNAL;
ALTER TABLE toasted_several ALTER COLUMN toasted_col1 SET STORAGE EXTERNAL;
ALTER TABLE toasted_several ALTER COLUMN toasted_col2 SET STORAGE EXTERNAL;
-INSERT INTO toasted_several(toasted_key) VALUES(repeat('9876543210', 1));
-SELECT pg_column_size(toasted_key) > 2^16 FROM toasted_several;
+INSERT INTO toasted_several(toasted_key) VALUES(repeat('9876543210', 269));
+SELECT pg_column_size(toasted_key) > 2^11 FROM toasted_several;
?column?
--
t
diff --git a/contrib/test_decoding/sql/toast.sql
b/contrib/test_decoding/sql/toast.sql
index ef11d2bfff..5cf6b87b3a 100644
--- a/contrib/test_decoding/sql/toast.sql
+++ b/contrib/test_decoding/sql/toast.sql
@@ -279,8 +279,8 @@ CREATE TABLE toasted_several (
ALTER TABLE toasted_several ALTER COLUMN toasted_col1 SET STORAGE EXTERNAL;
ALTER TABLE toasted_several ALTER COLUMN toasted_col2 SET STORAGE EXTERNAL;
-INSERT INTO toasted_several(toasted_key) VALUES(repeat('9876543210', 1));
-SELECT pg_column_size(toasted_key) > 2^16 FROM toasted_several;
+INSERT INTO toasted_several(toasted_key) VALUES(repeat('9876543210', 269));
+SELECT pg_column_size(toasted_key) > 2^11 FROM toasted_several;
SELECT regexp_replace(data, '^(.{100}).*(.{100})$', '\1..\2') FROM
pg_logical_slot_peek_changes('regression_slot', NULL, NULL, 'include-xids',
'0', 'skip-empty-xacts', '1');
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 1c4394abea..c4bc058e28 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -6900,6 +6900,7 @@ ATExecSetStorage(Relation rel, const char *colName, Node
*newValue, LOCKMODE loc
Form_pg_attribute attrtuple;
AttrNumber attnum;
ObjectAddress address;
+ ListCell *lc;
Assert(IsA(newValue, String));
storagemode = strVal(newValue);
@@ -6959,6 +6960,38 @@ ATExecSetStorage(Relation rel, const char *colName, Node
*newValue, LOCKMODE loc
heap_freetuple(tuple);
+ /*
+* Apply the change to indexes as well.
+*
+* XXX should probably use pg_index.indkey to find the right columns,
not
+* the column name
+*/
+ foreach(lc, RelationGetIndexList(rel))
+ {
+ Oid indexoid = lfirst_oid(lc);
+ Relationindrel;
+
+ indrel = index_open(indexoid, lockmode);
+
+ tuple = SearchSysCacheCopyAttName(RelationGetRelid(indrel),
colName);
+
+ if (HeapTupleIsValid(tuple))
+ {
+ attrtuple = (Form_pg_attribute) GETSTRUCT(tuple);
+ attrtuple->attstorage = newstorage;
+
+ CatalogTupleUpdate(attrelation, &tuple->t_self, tuple);
+
+ InvokeObjectPostAlterHook(RelationRelationId,
+
Re: pgbench - use pg logging capabilities
Bonjour Michaël,
Without looking at the context I thought that argv[0] was the program name,
which is not the case here. I put it back everywhere, including the DEBUG
message.
The variable names in Command are confusing IMO...
Somehow, yes. Note that there is a logic, it will indeed be the argv of
the called shell command… And I do not think it is the point of this patch
to solve this possible confusion.
+ pg_log_error("gaussian parameter must be at least "
+ "%f (not %f)", MIN_GAUSSIAN_PARAM, param);
I would keep all the error message strings to be on the same line.
That makes grepping for them easier on the same, and that's the usual
convention even if these are larger than 72-80 characters.
Ok. I also did other similar cases accordingly.
#ifdef DEBUG
Worth removing this ifdef?
Yep, especially as it is the only instance. Done.
+ pg_log_fatal("unexpected copy in result");
+ pg_log_error("%s", PQerrorMessage(con));
+ pg_log_fatal("cannot count number of branches");
+ pg_log_error("%s", PQerrorMessage(con));
These are inconsistent with the rest, why not combining both?
Ok, done.
I think that I would just remove the "debug" variable defined in
pgbench.c all together, and switch the messages for the duration and the
one in executeMetaCommand to use info-level logging..
Ok, done.
Patch v4 attached addresses all these points.
--
Fabien.diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index a1e0663c8b..11b701b359 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -70,7 +70,6 @@
#define M_PI 3.14159265358979323846
#endif
-
#define ERRCODE_UNDEFINED_TABLE "42P01"
/*
@@ -541,8 +540,6 @@ static ParsedScript sql_script[MAX_SCRIPTS]; /* SQL script files */
static int num_scripts; /* number of scripts in sql_script[] */
static int64 total_weight = 0;
-static int debug = 0; /* debug flag */
-
/* Builtin test scripts */
typedef struct BuiltinScript
{
@@ -787,14 +784,12 @@ strtoint64(const char *str, bool errorOK, int64 *result)
out_of_range:
if (!errorOK)
- fprintf(stderr,
-"value \"%s\" is out of range for type bigint\n", str);
+ pg_log_error("value \"%s\" is out of range for type bigint", str);
return false;
invalid_syntax:
if (!errorOK)
- fprintf(stderr,
-"invalid input syntax for type bigint: \"%s\"\n", str);
+ pg_log_error("invalid input syntax for type bigint: \"%s\"", str);
return false;
}
@@ -810,16 +805,14 @@ strtodouble(const char *str, bool errorOK, double *dv)
if (unlikely(errno != 0))
{
if (!errorOK)
- fprintf(stderr,
- "value \"%s\" is out of range for type double\n", str);
+ pg_log_error("value \"%s\" is out of range for type double", str);
return false;
}
if (unlikely(end == str || *end != '\0'))
{
if (!errorOK)
- fprintf(stderr,
- "invalid input syntax for type double: \"%s\"\n", str);
+ pg_log_error("invalid input syntax for type double: \"%s\"", str);
return false;
}
return true;
@@ -1149,7 +1142,8 @@ executeStatement(PGconn *con, const char *sql)
res = PQexec(con, sql);
if (PQresultStatus(res) != PGRES_COMMAND_OK)
{
- fprintf(stderr, "%s", PQerrorMessage(con));
+ pg_log_fatal("query failed: %s", PQerrorMessage(con));
+ pg_log_info("query was: %s", sql);
exit(1);
}
PQclear(res);
@@ -1164,8 +1158,8 @@ tryExecuteStatement(PGconn *con, const char *sql)
res = PQexec(con, sql);
if (PQresultStatus(res) != PGRES_COMMAND_OK)
{
- fprintf(stderr, "%s", PQerrorMessage(con));
- fprintf(stderr, "(ignoring this error and continuing anyway)\n");
+ pg_log_error("%s", PQerrorMessage(con));
+ pg_log_info("(ignoring this error and continuing anyway)");
}
PQclear(res);
}
@@ -1211,8 +1205,7 @@ doConnect(void)
if (!conn)
{
- fprintf(stderr, "connection to database \"%s\" failed\n",
- dbName);
+ pg_log_error("connection to database \"%s\" failed", dbName);
return NULL;
}
@@ -1230,8 +1223,8 @@ doConnect(void)
/* check to see that the backend connection was successfully made */
if (PQstatus(conn) == CONNECTION_BAD)
{
- fprintf(stderr, "connection to database \"%s\" failed:\n%s",
-dbName, PQerrorMessage(conn));
+ pg_log_error("connection to database \"%s\" failed: %s",
+ dbName, PQerrorMessage(conn));
PQfinish(conn);
return NULL;
}
@@ -1360,9 +1353,8 @@ makeVariableValue(Variable *var)
if (!strtodouble(var->svalue, true, &dv))
{
- fprintf(stderr,
- "malformed variable \"%s\" value: \"%s\"\n",
- var->name, var->svalue);
+ pg_log_error("malformed variable \"%s\" value: \"%s\"",
+ var->name, var->svalue);
return false;
}
setDoubleValue(&var->value, dv);
@@ -1425,8 +1417,7 @@ lookupCreateVariable(CState *st, const char *context, char *name)
*/
if (!valid_variable_name(name))
{
- fprintf(stderr, "%s: invalid variable name: \"%s\"\n",
- context, name);
+ pg_log_error("%s: invalid variable
Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)
On Sat, 4 Jan 2020 at 15:11, cary huang wrote: > > Hello Sawada and all > > I would like to elaborate more on Sehrope and Sawada's discussion on passing > NULL IV in "pg_cipher_encrypt/decrypt" functions during kmgr_wrap_key and > kmgr_unwrap_key routines in kmgr_utils.c. Openssl implements key wrap > according to RFC3394 as Sawada mentioned and passing NULL will make openssl > to use default IV, which equals to A6A6A6A6A6A6A6A6. I have confirmed this on > my side; A key wrapped with "NULL" IV can be unwrapped successfully with > IV=A6A6A6A6A6A6A6A6, and unwrap will fail if IV is set to anything else other > than NULL or A6A6A6A6A6A6A6A6. > Sehrope also suggested me not to use the fixed IV in order to avoid getting the same result from the same value. I'm researching it now. Also, currently it's using key wrap algorithm[1] but it accepts only multiple of 8 bytes as input. Since it's not good for some cases it's better to use key wrap with padding algorithm[2] instead, which seems available in OpenSSL 1.1.0 or later. > I would like to provide some comments on the encryption and decryption > routines provided by cipher_openssl.c in which cipher.c and kmgr_utils.c are > using. I see that "ossl_cipher_encrypt" calls "EVP_EncryptInit_ex" and > "EVP_EncryptUpdate" only to complete the encryption. Same thing applies to > decryption routines. According to my past experience with openssl and the > usages online, it is highly recommended to use "init-update-final" cycle to > complete the encryption and I see that the "final" part (EVP_EncryptFinal) is > missing. This call will properly handle the last block of data especially > when padding is taken into account. The functions still works now because the > input is encryption key and its size is multiple of each cipher block and no > padding is used. I think it will be safer to use the proper > "init-update-final" cycle for encryption/decryption Agreed. > > According to openssl EVP documentation, "EVP_EncryptUpdate" can be called > multiple times at different offset to the input data to be encrypted. I see > that "pg_cipher_encrypt" only calls "EVP_EncryptUpdate" once, which makes me > assume that the application should invoke "pg_cipher_encrypt" multiple times > until the entire data block is encrypted? I am asking because if we were to > use "EVP_EncryptFinal" to complete the encryption cycle, then it is better to > let "pg_cipher_encrypt" to figure out how many times "EVP_EncryptUpdate" > should be called and finalize it with "EVP_EncryptFinal" at last block. IIUC EVP_EncryptUpdate can encrypt the entire data block. EVP_EncryptFinal_ex encrypts any data that remains in a partial block. > > Lastly, I think we are missing a cleanup routine that calls > "EVP_CIPHER_CTX_free()" to free up the EVP_CIPHER_CTX when encryption is done. Right. While reading pgcrypto code I thought that it's better to change the cryptographic code (cipher.c) so that pgcrypto can use them instead of having duplicated code. I'm trying to change it so. [1] https://tools.ietf.org/html/rfc3394 [2] https://tools.ietf.org/html/rfc5649 Regards, -- Masahiko Sawadahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: [Proposal] Global temporary tables
On Mon, Jan 06, 2020 at 12:17:43PM +, Dean Rasheed wrote: On Mon, 6 Jan 2020 at 11:01, Tomas Vondra wrote: On Mon, Jan 06, 2020 at 01:04:15PM +0800, 曾文旌(义从) wrote: >2 We feel that gtt needs to maintain statistics, but there is no >agreement on what it will be done. > I certainly agree GTT needs to maintain statistics, otherwise it'll lead to poor query plans. +1 AFAIK the current patch stores the info in a hash table in a backend private memory, and I don't see how else to do that (e.g. storing it in a catalog would cause catalog bloat). It sounds like it needs a pair of system GTTs to hold the table and column statistics for other GTTs. One would probably have the same columns as pg_statistic, and the other just the relevant columns from pg_class. I can see it being useful for the user to be able to see these stats, so perhaps they could be UNIONed into the existing stats view. Hmmm, yeah. A "temporary catalog" (not sure if it can work exactly the same as GTT) storing pg_statistics data for GTTs might work, I think. It would not have the catalog bloat issue, which is good. I still think we'd need to integrate this with the regular pg_statistic catalogs somehow, so that people don't have to care about two things. I mean, extensions like hypopg do use pg_statistic data to propose indexes etc. and it would be nice if we don't make them more complicated. Not sure why we'd need a temporary version of pg_class, though? regards -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Greatest Common Divisor
Hello Robert, if (arg1 == PG_INT32_MIN) if (arg2 == 0 || arg2 == PG_INT32_MIN) And possibly a "likely" on the while. I don't think decoration the code with likely() and unlikely() all over the place is a very good idea. Odds are good that we'll end up with a bunch that are actually non-optimal, and nobody will ever figure it out because it's hard to figure out. My 0.02€: I'd tend to disagree. Modern pipelined processors can take advantage of speculative execution on branches, so if you know which branch is the more likely it can help. Obviously if you get it wrong it does not, but for the above cases it seems to me that they are rather straightforward. It also provides some "this case is expected to be exceptional" semantics to people reading the code. I have a hard time believing that we're going to be much worse off if we just write the code normally. I think that your point applies to more general programming in postgres, but this is not the context here. For low-level arithmetic code like this one, with tests and loops containing very few hardware instructions, I think that helping compiler optimizations is a good idea. Maybe in the "while" the compiler would assume that it is going to loop anyway by default, so it may be less useful there. -- Fabien.
Re: Error message inconsistency
On Sat, 6 Jul 2019 at 09:53, Amit Kapila wrote: > > On Mon, Jul 1, 2019 at 10:05 PM Alvaro Herrera wrote: > > > > Do we have an actual patch here? > > > > We have a patch, but it needs some more work like finding similar > places and change all of them at the same time and then change the > tests to adapt the same. > Hi all, Based on above discussion, I tried to find out all the places where we need to change error for "not null constraint". As Amit Kapila pointed out 1 place, I changed the error and adding modified patch. *What does this patch? * Before this patch, to display error of "not-null constraint", we were not displaying relation name in some cases so attached patch is adding relation name with the "not-null constraint" error in 2 places. I didn't changed out files of test suite as we haven't finalized error messages. I verified Robert's point of for partition tables also. With the error, we are adding relation name of "child table" and i think, it is correct. Please review attached patch and let me know feedback. Thanks and Regards Mahendra Singh Thalor EnterpriseDB: http://www.enterprisedb.com rationalize_constraint_error_messages_v2.patch Description: Binary data
Re: Ltree syntax improvement
Hi, This patch got mostly ignored since 2019-07 commitfest :-( The latest patch (sent by Nikita) does not apply because of a minor conflict in contrib/ltree/ltxtquery_io.c. I see the patch removes a small bit of ltree_plpython tests which would otherwise fail (with the "I don't know plpython" justification). Why not to instead update the tests to accept the new output? Or is it really the case that the case that we no longer need those tests? The patch also reworks some parts from "if" to "switch" statements. I agree switch statements are more readable, but maybe we should do this in two steps - first adopting the "switch" without changing the logic, and then making changes. But maybe that's an overkill. regards -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: [Proposal] Global temporary tables
po 6. 1. 2020 v 13:17 odesílatel Dean Rasheed napsal: > On Mon, 6 Jan 2020 at 11:01, Tomas Vondra > wrote: > > > > On Mon, Jan 06, 2020 at 01:04:15PM +0800, 曾文旌(义从) wrote: > > > > >2 We feel that gtt needs to maintain statistics, but there is no > > >agreement on what it will be done. > > > > > > > I certainly agree GTT needs to maintain statistics, otherwise it'll lead > > to poor query plans. > > +1 > > > AFAIK the current patch stores the info in a hash > > table in a backend private memory, and I don't see how else to do that > > (e.g. storing it in a catalog would cause catalog bloat). > > > > It sounds like it needs a pair of system GTTs to hold the table and > column statistics for other GTTs. One would probably have the same > columns as pg_statistic, and the other just the relevant columns from > pg_class. I can see it being useful for the user to be able to see > these stats, so perhaps they could be UNIONed into the existing stats > view. > +1 Pavel > Regards, > Dean >
Re: Setting min/max TLS protocol in clientside libpq
On Mon, Jan 6, 2020 at 7:02 AM Michael Paquier wrote: > > On Thu, Jan 02, 2020 at 09:46:44PM +, cary huang wrote: > > I agree with Arthur that it makes sense to check the validity of > > "conn->sslmaxprotocolversion" first before checking if it is larger > > than "conn->sslminprotocolversion" > > Here I don't agree. Why not just let OpenSSL handle things with > SSL_CTX_set_min_proto_version? We don't bother about that in the > backend code for that reason on top of keeping the code more simple > with less error handling. And things are cleaner when it comes to > this libpq patch by giving up with the INT_MIN hack. > > > A small suggestion here. I see that PG server defaults TLS min > > version to be TLSv1.2 and max version to none. So by default the > > server accepts TLSv1.2 and above. I think on the client side, it > > also makes sense to have the same defaults as the server. > > Yeah, that makes sense. Even more now that I have just removed > support for OpenSSL 0.9.8 and 1.0.0 ;) > > There could be an argument to lower down the default if we count for > backends built with OpenSSL versions older than libpq, but I am not > ready to buy that there would be many of those. Not having thought about it in much detail, but it's a fairly common scenario to have a much newer version of libpq (and the platform it's built on) than the server. E.g. a v12 libpq against a v9.6 postgres server is very common. For example, debian based systems will auto-upgrade your libpq, but not your server (for obvious reasons). And it's also quite common to upgrade platforms for the application much more frequently than the database server platform. -- Magnus Hagander Me: https://www.hagander.net/ Work: https://www.redpill-linpro.com/
Re: Setting min/max TLS protocol in clientside libpq
Magnus Hagander writes: > Not having thought about it in much detail, but it's a fairly common > scenario to have a much newer version of libpq (and the platform it's > built on) than the server. E.g. a v12 libpq against a v9.6 postgres > server is very common. For example, debian based systems will > auto-upgrade your libpq, but not your server (for obvious reasons). > And it's also quite common to upgrade platforms for the application > much more frequently than the database server platform. Yeah, there's a reason why we expect pg_dump and psql to function with ancient server versions. We shouldn't break this scenario with careless rejiggering of libpq's connection defaults. regards, tom lane
Re: pgsql: Add basic TAP tests for psql's tab-completion logic.
Christoph Berg writes: > I lost track of what bug is supposed to be where, so here's a summary > of the state at apt.postgresql.org: > PG13 head work on Debian unstable, buster, stretch. Cool. > Does not work on Ubuntu bionic, xenial. (Others not tested.) Hmm ... do we care? The test output seems to show that xenial's 3.1-20150325-1ubuntu2 libedit is completely broken. Maybe there's a way to work around that, but it's not clear to me that that'd be a useful expenditure of time. You're not really going to be building PG13 for that release are you? > Ubuntu bionic fails elsewhere [ apparently in 006_logical_decoding.pl ] Hmm. Not related to this thread then. But if that's reproducible, somebody should have a look. Maybe related to https://www.postgresql.org/message-id/CAA4eK1LMDx6vK8Kdw8WUeW1MjToN2xVffL2kvtHvZg17%3DY6QQg%40mail.gmail.com ??? (cc'ing Amit for that) Meanwhile, as to the point I was really concerned about, your table of current versions looks promising -- libedit's premature-dequote bug is evidently only in unstable and the last stable branch, and I presume the last-stables are still getting updated, since those have libedit versions that are less than a month old. I looked at Fedora's git repo and the situation seems similar over there. regards, tom lane
Re: Avoid full GIN index scan when possible
On Fri, Dec 27, 2019 at 04:36:14AM +0300, Nikita Glukhov wrote:
On 26.12.2019 4:59, Alexander Korotkov wrote:
I've tried to add patch #4 to comparison, but I've catch assertion
failure.
TRAP: FailedAssertion("key->includeNonMatching", File: "ginget.c",
Line: 1340)
There simply should be inverted condition in the assertion:
Assert(!key->includeNonMatching);
I have looked at v9 patch, and here is my review:
1. I agree with NULL-flag handling simplifications in ginNewScanKey(),
ginScanKeyAddHiddenEntry() extraction.
2. I also agree that usage of nrequired/nadditional in keyGetItem() is a more
natural solution to implement exclusion keys than my previous attempt of doing
that in scanGetKey().
But there are some questions:
Can we avoid referencing excludeOnly flag keyGetItem() by replacing these
references with !nrequired?
Maybe it would be better to move the whole block of keyGetItem() code
starting from the first loop over required keys and ending before the loop over
additional keys inside 'if (key->nrequired) { ... }'?
Can we avoid introducing excludeOnly flag by reusing searchMode and/or by
moving the initialization of nrequired/nadditional into ginNewScanKey()?
3. The following two times repeated NULL-filtering check looks too complicated
and needs to be refactored somehow:
- res = key->triConsistentFn(key);
+ if (key->excludeOnly &&
+ key->nuserentries < key->nentries &&
+ key->scanEntry[key->nuserentries]->queryCategory == GIN_CAT_NULL_KEY
&&
+ key->entryRes[key->nuserentries] == GIN_TRUE)
+ res = GIN_FALSE;
+ else
+ res = key->triConsistentFn(key);
For example, a special consistentFn() can be introduced for such NOT_NULL
scankeys. Or even a hidden separate one-entry scankey with a trivial
consistentFn() can be added instead of adding hidden entry.
4. forcedRecheck flag that was previously used for discarded empty ALL scankeys
is removed now. 0-entry exclusion keys can appear instead, and their
consistentFn() simply returns constant value. Could this lead to tangible
overhead in some cases (in comparison to forcedRecheck flag)?
5. A hidden GIN_CAT_EMPTY_QUERY is added only for the first empty ALL-scankey,
NULLs in other columns are filtered out with GIN_CAT_NULL_KEY. This looks like
asymmetric, and it leads to accelerations is some cases and slowdowns in others
(depending on NULL fractions and their correlations in columns).
The following test shows a significant performance regression of v9:
insert into t select array[i], NULL, NULL from generate_series(1, 100) i;
|Query time, ms
WHERE condition| master | v8 |v9
---+++-
a @> '{}' |224 |213 |212
a @> '{}' and b @> '{}' | 52 | 57 |255
a @> '{}' and b @> '{}' and c @> '{}' | 51 | 58 |290
In the older version of the patch I tried to do the similar things (initialize
only one NOT_NULL entry for the first column), but refused to do this in v8.
So, to avoid slowdowns relative to master, I can offer simply to add
GIN_CAT_EMPTY_QUERY entry for each column with empty ALL-keys if there are
no normal keys.
Yeah, I can confirm those results, although on my system the timings are
a bit different (I haven't tested v8):
|Query time, ms
WHERE condition| master |v9
---++-
a @> '{}' |610 |589
a @> '{}' and b @> '{}' |185 |665
a @> '{}' and b @> '{}' and c @> '{}' |185 |741
So that's something we probably need to address, perhaps by using the
GIN_CAT_EMPTY_QUERY entries as proposed.
I've also tested this on a database storing mailing lists archives with
a trigram index, and in that case the performance with short values gets
much better. The "messages" table has two text fields with a GIN trigram
index - subject and body, and querying them with short/long values works
like this:
WHERE| master |v9
--
subject LIKE '%aa%' AND body LIKE '%xx%' |4943 | 4052
subject LIKE '%aaa%' AND body LIKE '%xx%' | 10 |10
subject LIKE '%aa%' AND body LIKE '%xxx%' | 380 |13
subject LIKE '%aaa%' AND BODY LIKE '%xxx%' | 2 | 2
which seems fairly nice. I've done tests with individual columns, and
that seems to be working fine too.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Unicode normalization SQL functions
Peter Eisentraut wrote: > Also, there is a way to optimize the "is normalized" test for common > cases, described in UTR #15. For that we'll need an additional data > file from Unicode. In order to simplify that, I would like my patch > "Add support for automatically updating Unicode derived files" > integrated first. Would that explain that the NFC/NFKC normalization and "is normalized" check seem abnormally slow with the current patch, or should it be regarded independently of the other patch? For instance, testing 1 short ASCII strings: postgres=# select count(*) from (select md5(i::text) as t from generate_series(1,1) as i) s where t is nfc normalized ; count --- 1 (1 row) Time: 2573,859 ms (00:02,574) By comparison, the NFD/NFKD case is faster by two orders of magnitude: postgres=# select count(*) from (select md5(i::text) as t from generate_series(1,1) as i) s where t is nfd normalized ; count --- 1 (1 row) Time: 29,962 ms Although NFC/NFKC has a recomposition step that NFD/NFKD doesn't have, such a difference is surprising. I've tried an alternative implementation based on ICU's unorm2_isNormalized() /unorm2_normalize() functions (which I'm currently adding to the icu_ext extension to be exposed in SQL). With these, the 4 normal forms are in the 20ms ballpark with the above test case, without a clear difference between composed and decomposed forms. Independently of the performance, I've compared the results of the ICU implementation vs this patch on large series of strings with all normal forms and could not find any difference. Best regards, -- Daniel Vérité PostgreSQL-powered mailer: http://www.manitou-mail.org Twitter: @DanielVerite
Re: Recognizing superuser in pg_hba.conf
So it's not clear to me whether we have any meeting of the minds on wanting this patch. In the meantime, though, the cfbot reports that the patch breaks the ssl tests. Why is that? regards, tom lane
Re: Allow WHEN in INSTEAD OF triggers
On Sat, 28 Dec 2019 at 16:45, David Fetter wrote: > > On Sat, Dec 28, 2019 at 12:12:30AM -0300, Alvaro Herrera wrote: > > On 2019-Dec-28, David Fetter wrote: > > > > > While noodling around with an upcoming patch to remove user-modifiable > > > RULEs, I noticed that WHEN conditions were disallowed from INSTEAD OF > > > triggers for no discernible reason. This patch removes that > > > restriction. > > > > If you want to remove the restriction, your patch should add some test > > cases that show it working. > > Tests added :) > I too think this is a bad idea. Doing nothing if the trigger's WHEN condition isn't satisfied is not consistent with the way other types of trigger work -- with any other type of trigger, if the WHEN condition doesn't evaluate to true, the query goes ahead as if the trigger hadn't been there. So the most consistent thing to do would be to attempt an auto-update if the trigger isn't fired, and that leads to a whole other world of pain (e.g., you'd need 2 completely different query plans for the 2 cases, and more if you had views on top of other views). The SQL spec explicitly states that INSTEAD OF triggers on views should not have WHEN clauses, and for good reason. There are cases where it makes sense to deviate from the spec, but I don't think this is one of them. Regards, Dean
Re: Removing pg_pltemplate and creating "trustable" extensions
On Thu, Nov 7, 2019 at 2:13 PM Stephen Frost wrote: > I do not agree that we should just shift to using default roles instead > of adding new options to GRANT because of an entirely internal > implementation detail that we could fix (and should, as I've said for > probably 10 years now...). +1. I'm not sure that Tom's latest design idea is a bad one, but I strongly suspect that wrapping ourselves around the axle to work around our unwillingness to widen a 16-bit quantity to 32 bits (or a 32 bit quantity to 64 bits) is a bad idea. Perhaps there are also design ideas that we should consider, like separating "basic" privileges and "extended" privileges or coming up with some altogether new and better representation. But limiting ourselves to 4 more privileges ever cannot be the right solution. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: proposal: minscale, rtrim, btrim functions for numeric
Pavel Stehule writes: > út 10. 12. 2019 v 13:56 odesílatel Karl O. Pinc napsal: >> I'm marking it ready for a committer. > Thank you for review Pushed with minor adjustments. Notably, I didn't like having get_min_scale() depend on its callers having stripped trailing zeroes to avoid getting into a tight infinite loop. That's just trouble waiting to happen, especially since non-stripped numerics are seldom seen in practice (ones coming into the SQL-level functions should never look like that, ie the strip_var calls you had are almost certainly dead code). If we did have a code path where the situation could occur, and somebody forgot the strip_var call, the omission could easily escape notice. So I got rid of the strip_var calls and made get_min_scale() defend itself against the case. It's hardly any more code, and it should be a shade faster than strip_var anyway. regards, tom lane
Re: Assert failure due to "drop schema pg_temp_3 cascade" for temporary tables and \d+ is not showing any info after drooping temp table schema
On Sun, Jan 5, 2020 at 8:42 PM Michael Paquier wrote:
> The behavior of the code in 246a6c8 has changed so as a non-existing
> temporary namespace is considered as not in use, in which case
> autovacuum would consider this relation to be orphaned, and it would
> then try to drop it. Anyway, just a revert of the patch is not a good
> idea either, because keeping around the old behavior allows any user
> to create orphaned relations that autovacuum would just ignore in
> 9.4~10, leading to ultimately a forced shutdown of the instance as no
> cleanup can happen if this goes unnoticed. This also puts pg_class
> into an inconsistent state as pg_class entries would include
> references to a namespace that does not exist for sessions still
> holding its own references to myTempNamespace/myTempToastNamespace.
I'm not arguing for a revert of 246a6c8. I think we should just change this:
ereport(LOG,
(errmsg("autovacuum: dropping orphan
temp table \"%s.%s.%s\"",
get_database_name(MyDatabaseId),
get_namespace_name(classForm->relnamespace),
NameStr(classForm->relname;
To look more like:
char *nspname = get_namespace_name(classForm->relnamespace);
if (nspname != NULL)
ereport(..."autovacuum: dropping orphan temp table \"%s.%s.%s\"...)
else
ereport(..."autovacuum: dropping orphan temp table with OID %u")
If we do that, then I think we can just revert
a052f6cbb84e5630d50b68586cecc127e64be639 completely. As a side
benefit, this would also provide some insurance against other
possibly-problematic situations, like a corrupted pg_class row with a
garbage value in the relnamespace field, which is something I've seen
multiple times in the field.
I can't quite understand your comments about why we shouldn't do that,
but the reported bug is just a null pointer reference. Incredibly,
autovacuum.c seems to have been using get_namespace_name() without a
null check since 2006, so it's not really the fault of your patch as I
had originally thought. I wonder how in the world we've managed to get
away with it for as long as we have.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Re: Assert failure due to "drop schema pg_temp_3 cascade" for temporary tables and \d+ is not showing any info after drooping temp table schema
Robert Haas writes: > I'm not arguing for a revert of 246a6c8. I think we should just change this: > ... > To look more like: > char *nspname = get_namespace_name(classForm->relnamespace); > if (nspname != NULL) >ereport(..."autovacuum: dropping orphan temp table \"%s.%s.%s\"...) > else >ereport(..."autovacuum: dropping orphan temp table with OID %u") > If we do that, then I think we can just revert > a052f6cbb84e5630d50b68586cecc127e64be639 completely. +1 to both of those --- although I think we could still provide the table name in the null-nspname case. > autovacuum.c seems to have been using get_namespace_name() without a > null check since 2006, so it's not really the fault of your patch as I > had originally thought. I wonder how in the world we've managed to get > away with it for as long as we have. Maybe we haven't. It's not clear that infrequent autovac crashes would get reported to us, or that we'd successfully find the cause if they were. I think what you propose above is a back-patchable bug fix. regards, tom lane
Re: Recognizing superuser in pg_hba.conf
On 06/01/2020 17:03, Tom Lane wrote: > So it's not clear to me whether we have any meeting of the minds > on wanting this patch. In the meantime, though, the cfbot > reports that the patch breaks the ssl tests. Why is that? I have no idea. I cannot reproduce the failure locally. -- Vik Fearing
Re: [PATCH] fix a performance issue with multiple logical-decoding walsenders
Pierre Ducroquet writes: > Attached to this email is a patch with better comments regarding the > XLogSendLogical change. Hi, This patch entirely fails to apply for me (and for the cfbot too). It looks like (a) it's missing a final newline and (b) all the tabs have been mangled into spaces, and not correctly mangled either. I could probably reconstruct a workable patch if I had to, but it seems likely that it'd be easier for you to resend it with a little more care about attaching an unmodified attachment. As for the question of back-patching, it seems to me that it'd likely be reasonable to put this into v12, but probably not further back. There will be no interest in back-patching commit cfdf4dc4f, and it seems like the argument for this patch is relatively weak without that. regards, tom lane
Re: proposal: minscale, rtrim, btrim functions for numeric
po 6. 1. 2020 v 18:22 odesílatel Tom Lane napsal: > Pavel Stehule writes: > > út 10. 12. 2019 v 13:56 odesílatel Karl O. Pinc napsal: > >> I'm marking it ready for a committer. > > > Thank you for review > > Pushed with minor adjustments. Notably, I didn't like having > get_min_scale() depend on its callers having stripped trailing zeroes > to avoid getting into a tight infinite loop. That's just trouble > waiting to happen, especially since non-stripped numerics are seldom > seen in practice (ones coming into the SQL-level functions should > never look like that, ie the strip_var calls you had are almost > certainly dead code). If we did have a code path where the situation > could occur, and somebody forgot the strip_var call, the omission > could easily escape notice. So I got rid of the strip_var calls and > made get_min_scale() defend itself against the case. It's hardly > any more code, and it should be a shade faster than strip_var anyway. > Thank you very much Maybe this issue was part of ToDo list Pavel > regards, tom lane >
Re: Removing pg_pltemplate and creating "trustable" extensions
Robert Haas writes: > On Thu, Nov 7, 2019 at 2:13 PM Stephen Frost wrote: >> I do not agree that we should just shift to using default roles instead >> of adding new options to GRANT because of an entirely internal >> implementation detail that we could fix (and should, as I've said for >> probably 10 years now...). > +1. > I'm not sure that Tom's latest design idea is a bad one, but I > strongly suspect that wrapping ourselves around the axle to work > around our unwillingness to widen a 16-bit quantity to 32 bits (or a > 32 bit quantity to 64 bits) is a bad idea. Perhaps there are also > design ideas that we should consider, like separating "basic" > privileges and "extended" privileges or coming up with some altogether > new and better representation. But limiting ourselves to 4 more > privileges ever cannot be the right solution. So, is that actually an objection to the current proposal, or just an unrelated rant? If we think that a privilege bit on databases can actually add something useful to this design, the fact that it moves us one bit closer to needing to widen AclMode doesn't seem like a serious objection. But I don't actually see what such a bit will buy for this purpose. A privilege bit on a database is presumably something that can be granted or revoked by the database owner, and I do not see that we want any such behavior for extension installation privileges. regards, tom lane
Re: could not access status of transaction
On Sun, Jan 5, 2020 at 11:00 PM chenhj wrote: > According to above information, the flags of the heap page (163363) with the > problem tuple (163363, 9) is 0x0001 (HAS_FREE_LINES), that is, ALL_VISIBLE is > not set. > > However, according hexdump content of the corresponding vm file, that > block(location is 9F88 + 6bit) has set VISIBILITYMAP_ALL_FROZEN and > VISIBILITYMAP_ALL_VISIBLE flags. That is, the heap file and the vm file are > inconsistent. That's not supposed to happen, and represents data corruption. Your previous report of a too-old xmin surviving in the heap is also corruption. There is no guarantee that both problems have the same cause, but suppose they do. One possibility is that a write to the heap page may have gotten lost or undone. Suppose that, while this page was in shared_buffers, VACUUM came through and froze it, setting the bits in the VM and later truncating CLOG. Then, suppose that when that page was evicted from shared_buffers, it didn't really get written back to disk, or alternatively it did, but then later somehow the old version reappeared. I think that would produce these symptoms. I think that bad hardware could cause this, or running two copies of the server on the same data files at the same time, or maybe some kind of filesystem-related flakiness, especially if, for example, you are using a network filesystem like NFS, or maybe a broken iSCSI stack. There is also no reason it couldn't be a bug in PostgreSQL itself, although if we lost page writes routinely somebody would surely have noticed by now. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: [PATCH] fix a performance issue with multiple logical-decoding walsenders
On Monday, January 6, 2020 6:57:33 PM CET Tom Lane wrote:
> Pierre Ducroquet writes:
> > Attached to this email is a patch with better comments regarding the
> > XLogSendLogical change.
>
> Hi,
> This patch entirely fails to apply for me (and for the cfbot too).
> It looks like (a) it's missing a final newline and (b) all the tabs
> have been mangled into spaces, and not correctly mangled either.
> I could probably reconstruct a workable patch if I had to, but
> it seems likely that it'd be easier for you to resend it with a
> little more care about attaching an unmodified attachment.
>
> As for the question of back-patching, it seems to me that it'd
> likely be reasonable to put this into v12, but probably not
> further back. There will be no interest in back-patching
> commit cfdf4dc4f, and it seems like the argument for this
> patch is relatively weak without that.
>
> regards, tom lane
Hi
My deepest apologies for the patch being broken, I messed up when transferring
it between my computers after altering the comments. The verbatim one attached
to this email applies with no issue on current HEAD.
The patch regarding PostmasterIsAlive is completely pointless since v12 where
the function was rewritten, and was included only to help reproduce the issue
on older versions. Back-patching the walsender patch further than v12 would
imply back-patching all the machinery introduced for PostmasterIsAlive
(9f09529952) or another intrusive change there, a too big risk indeed.
Regards
Pierre
diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
index 6e80e67590..4b57db6fc2 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -2774,7 +2774,13 @@ XLogSendLogical(void)
{
XLogRecord *record;
char *errm;
- XLogRecPtr flushPtr;
+
+ /*
+ * We'll use the current flush point to determine whether we've caught up.
+ * This variable is static in order to cache it accross calls. This caching
+ * is needed because calling GetFlushRecPtr needs to acquire an expensive lock.
+ */
+ static XLogRecPtr flushPtr = InvalidXLogRecPtr;
/*
* Don't know whether we've caught up yet. We'll set WalSndCaughtUp to
@@ -2791,11 +2797,6 @@ XLogSendLogical(void)
if (errm != NULL)
elog(ERROR, "%s", errm);
- /*
- * We'll use the current flush point to determine whether we've caught up.
- */
- flushPtr = GetFlushRecPtr();
-
if (record != NULL)
{
/*
@@ -2808,7 +2809,15 @@ XLogSendLogical(void)
sentPtr = logical_decoding_ctx->reader->EndRecPtr;
}
- /* Set flag if we're caught up. */
+ /* Initialize flushPtr if needed */
+ if (flushPtr == InvalidXLogRecPtr)
+ flushPtr = GetFlushRecPtr();
+
+ /* If EndRecPtr is past our flushPtr, we must update it to know if we caught up */
+ if (logical_decoding_ctx->reader->EndRecPtr >= flushPtr)
+ flushPtr = GetFlushRecPtr();
+
+ /* If EndRecPtr is still past our flushPtr, it means we caught up */
if (logical_decoding_ctx->reader->EndRecPtr >= flushPtr)
WalSndCaughtUp = true;
Re: pgsql: Add basic TAP tests for psql's tab-completion logic.
Re: Tom Lane 2020-01-06 <3764.1578323...@sss.pgh.pa.us> > > Does not work on Ubuntu bionic, xenial. (Others not tested.) > > Hmm ... do we care? The test output seems to show that xenial's > 3.1-20150325-1ubuntu2 libedit is completely broken. Maybe there's > a way to work around that, but it's not clear to me that that'd > be a useful expenditure of time. You're not really going to be > building PG13 for that release are you? xenial (16.04) is a LTS release with support until 2021-04, and the current plan was to support it. I now realize that's semi-close to the 13 release date, but so far we have tried to really support all PG-Distro combinations. I could probably arrange for that test to be disabled when building for xenial, but it'd be nice if there were a configure switch or environment variable for it so we don't have to invent it. > > Ubuntu bionic fails elsewhere [ apparently in 006_logical_decoding.pl ] > > Hmm. Not related to this thread then. But if that's reproducible, It has been failing with the same output since Jan 2, 2020, noon. > somebody should have a look. Maybe related to > > https://www.postgresql.org/message-id/CAA4eK1LMDx6vK8Kdw8WUeW1MjToN2xVffL2kvtHvZg17%3DY6QQg%40mail.gmail.com The only git change in that build was d207038053837 "Fix running out of file descriptors for spill files." Christoph
Re: Removing pg_pltemplate and creating "trustable" extensions
Greetings, * Tom Lane (t...@sss.pgh.pa.us) wrote: > Robert Haas writes: > > On Thu, Nov 7, 2019 at 2:13 PM Stephen Frost wrote: > >> I do not agree that we should just shift to using default roles instead > >> of adding new options to GRANT because of an entirely internal > >> implementation detail that we could fix (and should, as I've said for > >> probably 10 years now...). > > > +1. > > > I'm not sure that Tom's latest design idea is a bad one, but I > > strongly suspect that wrapping ourselves around the axle to work > > around our unwillingness to widen a 16-bit quantity to 32 bits (or a > > 32 bit quantity to 64 bits) is a bad idea. Perhaps there are also > > design ideas that we should consider, like separating "basic" > > privileges and "extended" privileges or coming up with some altogether > > new and better representation. But limiting ourselves to 4 more > > privileges ever cannot be the right solution. > > So, is that actually an objection to the current proposal, or just > an unrelated rant? It strikes me as related since using a bit was one of the objections to using the GRANT-a-privilege approach. > If we think that a privilege bit on databases can actually add something > useful to this design, the fact that it moves us one bit closer to needing > to widen AclMode doesn't seem like a serious objection. But I don't > actually see what such a bit will buy for this purpose. A privilege bit > on a database is presumably something that can be granted or revoked by > the database owner, and I do not see that we want any such behavior for > extension installation privileges. Given that extensions are database-level objects, I ask: why not? Database owners are already able to create schema, and therefore to create any object inside an extension that doesn't require a superuser to create, why not let them also create the framework for those objects to exist in, in the form of an extension? When it comes to *trusted* extensions, I would view those in basically the exact same way we view *trusted* languages- that is, if they're trusted, then they can't be used to bypass the privilege system that exists in PG, nor can they be used to operate directly on the filesystem or open sockets, etc, at least- not without further checks. For example, I would think postgres_fdw could be a 'trusted' extension, since it only allows superusers to create FDWs, and you can't create a server unless you have rights on the FDW. When it comes to *untrusted* extensions, we could limit those to being only installable by superusers, in the same way that functions in untrusted languages are only able to be created by superusers (except, perhaps as part of a trusted extension, assuming we can work through this). Now, I'm no fan of growing the set of things that only a superuser can do, but I don't see that as being what we're doing here because we're (hopefully) going to at least make it so that non-superusers can do some things (create trusted extensions) that used to only be possible for superusers to do, even if it still requires being a superuser to create untrusted extensions. If someone comes up with a really strong use-case then for allowing non-superusers to create untrusted extensions, then we could consider how to enable that and maybe a default role makes sense for that specific case, but I don't think anyone's really made that case and I certainly don't think we want the privilege to create trusted extensions and the privilege to create untrusted ones to be the same- it's clear made that users will want to grant out those abilities independently. Thanks, Stephen signature.asc Description: PGP signature
Re: [PATCH] fix a performance issue with multiple logical-decoding walsenders
On Mon, Jan 6, 2020 at 7:57 PM Pierre Ducroquet wrote: > > On Monday, January 6, 2020 6:57:33 PM CET Tom Lane wrote: > > Pierre Ducroquet writes: > > > Attached to this email is a patch with better comments regarding the > > > XLogSendLogical change. > > > > Hi, > > This patch entirely fails to apply for me (and for the cfbot too). > > It looks like (a) it's missing a final newline and (b) all the tabs > > have been mangled into spaces, and not correctly mangled either. > > I could probably reconstruct a workable patch if I had to, but > > it seems likely that it'd be easier for you to resend it with a > > little more care about attaching an unmodified attachment. > > > > As for the question of back-patching, it seems to me that it'd > > likely be reasonable to put this into v12, but probably not > > further back. There will be no interest in back-patching > > commit cfdf4dc4f, and it seems like the argument for this > > patch is relatively weak without that. > > > > regards, tom lane > > Hi > > My deepest apologies for the patch being broken, I messed up when transferring > it between my computers after altering the comments. The verbatim one attached > to this email applies with no issue on current HEAD. > The patch regarding PostmasterIsAlive is completely pointless since v12 where > the function was rewritten, and was included only to help reproduce the issue > on older versions. Back-patching the walsender patch further than v12 would > imply back-patching all the machinery introduced for PostmasterIsAlive > (9f09529952) or another intrusive change there, a too big risk indeed. +1, backpatch to v12 looks sensible.
Re: [PATCH] fix a performance issue with multiple logical-decoding walsenders
Hi, I spent a little bit of time trying to explain the problem we are facing clearly, and provide a reproducible benchmark. So here it is. What triggered our investigation is that we have a PostgreSQL cluster containing about 15 databases, most of them being used as sources for logical replication. This means we have about as many WAL senders active on the cluster at the same time. What we saw was that we had very high spikes of CPU activity, with very high level of SYS (up to 50% of the whole system was SYS, with 100% load on all CPUs) on the server, which is a dual Xeon Silver 4110, so 16 cores, 32 threads. That seemed insane as our usual load isn't that high on average (like 20% total cpu use, with ~ 1% of SYS), and mostly on 2 of those 15 databases. This mostly occured when creating an index, or doing batch updates, copys, etc. And all WAL senders consumed about the same amount, even those connected to databases where nothing happened. And while testing, we noticed things were worse with PG 12 than with PG 10 (that was the cause for the first patch Pierre posted, which was more of a way to get PostmasterIsAlive out of the way for PG 10 to get the same behaviour on both databases and confirm what was different between the two versions). So that was what the first benchmark we did, what Pierre posted a few days ago. With the second patch (reducing calls to GetFlushRecPtr), on PostgreSQL 12, with statements affecting lots of records at once, we managed to reduce the WAL senders' consumption by a factor of 15 (if the patch is correct of course). SYS was down to more sensible (near 0) values. WAL senders for databases which had no decoding to do didn't consume that much anymore, only the one connected to the database doing the work used a lot of CPU, but that's expected. This solves the problem we are facing. Without this, we won't be able to upgrade to PG 12, as the impact of GetFlushRecPtr is even worse than with PG 10. I've now tried to measure the impact of the patch on a more evenly balanced activity on several databases, where the contention on GetFlushRecPtr is less severe, to see if there are wins in all cases. Simple scripts to test this are provided as an attachment. Just set the "max" environment variable to the amount of databases/WAL senders you want, and then run create_logical.sh (creates the databases and the slots), then connect_logical.sh (connects pg_recvlogical processes to these slots), and run_stress.sh (runs a pgbench on each database, each doing 10 transactions, all in parallel). drop_logical.sh does the cleanup. Sorry, they are very basic... Here are the results on patched/unpatched PG 12. You have the messages from all pgbenchs, then the consumption of all WAL senders at the end of a run. As you can see, pgbench runs are ~ 20% faster. That's because with unpatched, we are CPU starved (the server is 100% CPU), which we aren't with patched. WAL senders needed about half the CPU in the patched case as shown by pidstat. UNPATCHED: transaction type: scaling factor: 1 query mode: simple number of clients: 1 number of threads: 1 number of transactions per client: 10 number of transactions actually processed: 10/10 latency average = 1.943 ms tps = 514.796454 (including connections establishing) tps = 514.805610 (excluding connections establishing) transaction type: scaling factor: 1 query mode: simple number of clients: 1 number of threads: 1 number of transactions per client: 10 number of transactions actually processed: 10/10 latency average = 1.949 ms tps = 513.130790 (including connections establishing) tps = 513.168135 (excluding connections establishing) transaction type: scaling factor: 1 query mode: simple number of clients: 1 number of threads: 1 number of transactions per client: 10 number of transactions actually processed: 10/10 latency average = 1.950 ms tps = 512.946425 (including connections establishing) tps = 512.961746 (excluding connections establishing) transaction type: scaling factor: 1 query mode: simple number of clients: 1 number of threads: 1 number of transactions per client: 10 number of transactions actually processed: 10/10 latency average = 1.951 ms tps = 512.643065 (including connections establishing) tps = 512.678765 (excluding connections establishing) transaction type: scaling factor: 1 query mode: simple number of clients: 1 number of threads: 1 number of transactions per client: 10 number of transactions actually processed: 10/10 latency average = 1.953 ms tps = 512.159794 (including connections establishing) tps = 512.178075 (excluding connections establishing) transaction type: scaling factor: 1 query mode: simple number of clients: 1 number of threads: 1 number of transactions per client: 10 number of transactions actually processed: 10/10 latency average = 1.953 ms tps = 512.024962 (including connections establishing) tps =
Re: NOT IN subquery optimization
Hi Andrey, Thanks for the comment! The unimproved cases you mentioned all fall into the category “correlated subquery”. This category is explicitly disallowed by existing code to convert to join in convert_ANY_sublink_to_join: /* * The sub-select must not refer to any Vars of the parent query. (Vars of * higher levels should be okay, though.) */ if (contain_vars_of_level((Node *) subselect, 1)) return NULL; I think this is also the reason why hashed subplan is not used for such subqueries. It's probably not always safe to convert a correlated subquery to join. We need to find out/prove when it’s safe/unsafe to convert such ANY subquery if we were to do so. Regards, --- Zheng Li AWS, Amazon Aurora PostgreSQL On 1/5/20, 1:12 AM, "Andrey Lepikhov" wrote: At the top of the thread your co-author argued the beginning of this work with "findings about the performance of PostgreSQL, MySQL, and Oracle on various subqueries:" https://antognini.ch/2017/12/how-well-a-query-optimizer-handles-subqueries/ I launched this test set with your "not_in ..." patch. Your optimization improves only results of D10-D13 queries. Nothing has changed for bad plans of the E20-E27 and F20-F27 queries. For example, we can replace E20 query: SELECT * FROM large WHERE n IN (SELECT n FROM small WHERE small.u = large.u); - execution time: 1370 ms, by SELECT * FROM large WHERE EXISTS (SELECT n,u FROM small WHERE (small.u = large.u) AND (large.n = small.n )) AND n IS NOT NULL; - execution time: 0.112 ms E21 query: SELECT * FROM large WHERE n IN (SELECT nn FROM small WHERE small.u = large.u); - 1553 ms, by SELECT * FROM large WHERE EXISTS (SELECT nn FROM small WHERE (small.u = large.u) AND (small.nn = large.n)); - 0.194 ms F27 query: SELECT * FROM large WHERE nn NOT IN (SELECT nn FROM small WHERE small.nu = large.u); - 1653.048 ms, by SELECT * FROM large WHERE NOT EXISTS (SELECT nn,nu FROM small WHERE (small.nu = large.u) AND (small.nn = large.nn)); - 274.019 ms Are you planning to make another patch for these cases? Also i tried to increase work_mem up to 2GB: may be hashed subqueries can improve situation? But hashing is not improved execution time of the queries significantly. On your test cases (from the comments of the patch) the subquery hashing has the same execution time with queries No.13-17. At the queries No.1-12 it is not so slow as without hashing, but works more slowly (up to 3 orders) than NOT IN optimization. On 12/2/19 9:25 PM, Li, Zheng wrote: > Here is the latest rebased patch. -- Andrey Lepikhov Postgres Professional https://postgrespro.com The Russian Postgres Company
Re: bitmaps and correlation
Find attached cleaned up patch.
For now, I updated the regress/expected/, but I think the test maybe has to be
updated to do what it was written to do.
>From 36f547d69b8fee25869d6ef3ef26d327a8ba1205 Mon Sep 17 00:00:00 2001
From: Justin Pryzby
Date: Tue, 1 Jan 2019 16:17:28 -0600
Subject: [PATCH v1] Use correlation statistic in costing bitmap scans..
Same as for an index scan, a correlated bitmap which accesses pages across a
small portion of the table should have a cost estimate much less than an
uncorrelated scan (like modulus) across the entire length of the table, the
latter having a high component of random access.
Note, Tom points out that there are cases where a column could be
tightly-"clumped" without being highly-ordered. Since we have correlation
already, we use that until such time as someone implements a new statistic for
clumpiness. This patch only intends to make costing of bitmap heap scan on par
with the same cost of index scan without bitmap.
---
contrib/postgres_fdw/expected/postgres_fdw.out | 15 ++--
src/backend/optimizer/path/costsize.c | 94 +-
src/backend/optimizer/path/indxpath.c | 10 ++-
src/include/nodes/pathnodes.h | 3 +
src/include/optimizer/cost.h | 2 +-
src/test/regress/expected/create_index.out | 16 ++---
src/test/regress/expected/join.out | 18 +++--
src/test/regress/sql/create_index.sql | 8 ++-
8 files changed, 118 insertions(+), 48 deletions(-)
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index c915885..fb4c7f2 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -2257,11 +2257,12 @@ SELECT * FROM ft1, ft2, ft4, ft5, local_tbl WHERE ft1.c1 = ft2.c1 AND ft1.c2 = f
-> Foreign Scan on public.ft1
Output: ft1.c1, ft1.c2, ft1.c3, ft1.c4, ft1.c5, ft1.c6, ft1.c7, ft1.c8, ft1.*
Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1" WHERE (("C 1" < 100)) FOR UPDATE
- -> Materialize
+ -> Sort
Output: ft2.c1, ft2.c2, ft2.c3, ft2.c4, ft2.c5, ft2.c6, ft2.c7, ft2.c8, ft2.*
+ Sort Key: ft2.c1
-> Foreign Scan on public.ft2
Output: ft2.c1, ft2.c2, ft2.c3, ft2.c4, ft2.c5, ft2.c6, ft2.c7, ft2.c8, ft2.*
- Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1" WHERE (("C 1" < 100)) ORDER BY "C 1" ASC NULLS LAST FOR UPDATE
+ Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1" WHERE (("C 1" < 100)) FOR UPDATE
-> Sort
Output: ft4.c1, ft4.c2, ft4.c3, ft4.*
Sort Key: ft4.c1
@@ -2276,7 +2277,7 @@ SELECT * FROM ft1, ft2, ft4, ft5, local_tbl WHERE ft1.c1 = ft2.c1 AND ft1.c2 = f
Remote SQL: SELECT c1, c2, c3 FROM "S 1"."T 4" FOR UPDATE
-> Index Scan using local_tbl_pkey on public.local_tbl
Output: local_tbl.c1, local_tbl.c2, local_tbl.c3, local_tbl.ctid
-(47 rows)
+(48 rows)
SELECT * FROM ft1, ft2, ft4, ft5, local_tbl WHERE ft1.c1 = ft2.c1 AND ft1.c2 = ft4.c1
AND ft1.c2 = ft5.c1 AND ft1.c2 = local_tbl.c1 AND ft1.c1 < 100 AND ft2.c1 < 100 FOR UPDATE;
@@ -3318,10 +3319,12 @@ select c2, sum from "S 1"."T 1" t1, lateral (select sum(t2.c1 + t1."C 1") sum fr
Sort Key: t1.c2
-> Nested Loop
Output: t1.c2, qry.sum
- -> Index Scan using t1_pkey on "S 1"."T 1" t1
+ -> Bitmap Heap Scan on "S 1"."T 1" t1
Output: t1."C 1", t1.c2, t1.c3, t1.c4, t1.c5, t1.c6, t1.c7, t1.c8
- Index Cond: (t1."C 1" < 100)
+ Recheck Cond: (t1."C 1" < 100)
Filter: (t1.c2 < 3)
+ -> Bitmap Index Scan on t1_pkey
+ Index Cond: (t1."C 1" < 100)
-> Subquery Scan on qry
Output: qry.sum, t2.c1
Filter: ((t1.c2 * 2) = qry.sum)
@@ -3329,7 +3332,7 @@ select c2, sum from "S 1"."T 1" t1, lateral (select sum(t2.c1 + t1."C 1") sum fr
Output: (sum((t2.c1 + t1."C 1"))), t2.c1
Relations: Aggregate on (public.ft2 t2)
Remote SQL: SELECT sum(("C 1" + $1::integer)), "C 1" FROM "S 1"."T 1" GROUP BY 2
-(16 rows)
+(18 rows)
select c2, sum from "S 1"."T 1" t1, lateral (select sum(t2.c1 + t1."C 1") sum from ft2 t2 group by t2.c1) qry where t1.c2 * 2 = qry.sum and t1.c2 <
Re: pg_basebackup fails on databases with high OIDs
On Mon, Jan 6, 2020 at 9:31 AM Julien Rouhaud wrote: > > On Mon, Jan 6, 2020 at 9:21 AM Michael Paquier wrote: > > > > On Mon, Jan 06, 2020 at 09:07:26AM +0100, Peter Eisentraut wrote: > > > This is a new bug in PG12. When you have a database with an OID above > > > INT32_MAX (signed), then pg_basebackup fails thus: > > > > Yep. Introduced by 6b9e875. > > Indeed. Yeah, clearly :/ > > > pg_basebackup: error: could not get write-ahead log end position from > > > server: ERROR: value "30" is out of range for type integer > > > > > > The cause appears to be commit 6b9e875f7286d8535bff7955e5aa3602e188e436. > > > > > > A possible fix is attached. An alternative to using > > > OidInputFunctionCall() > > > would be exporting something like oidin_subr(). > > > > I think that you would save yourself from a lot of trouble if you do > > the latter with a subroutine. Not quite like that based on the > > process context where the call is done, but remember 21f428eb.. > > +0.5 to avoid calling OidInputFunctionCall() Or just directly using atol() instead of atoi()? Well maybe not directly but in a small wrapper that verifies it's not bigger than an unsigned? Unlike in cases where we use oidin etc, we are dealing with data that is "mostly trusted" here, aren't we? Meaning we could call atol() on it, and throw an error if it overflows, and be done with it? Subdirectories in the data directory aren't exactly "untrusted enduser data"... I agree with the feelings that calling OidInputFunctionCall() from this context leaves me slightly irked. -- Magnus Hagander Me: https://www.hagander.net/ Work: https://www.redpill-linpro.com/
Re: Greatest Common Divisor
On Mon, Jan 6, 2020 at 6:52 AM Fabien COELHO wrote: > > > Hello Robert, > > >> if (arg1 == PG_INT32_MIN) > >> if (arg2 == 0 || arg2 == PG_INT32_MIN) > >> > >> And possibly a "likely" on the while. > > > > I don't think decoration the code with likely() and unlikely() all > > over the place is a very good idea. > > > Odds are good that we'll end up with a bunch that are actually > > non-optimal, and nobody will ever figure it out because it's hard to > > figure out. > > My 0.02€: I'd tend to disagree. > > Modern pipelined processors can take advantage of speculative execution on > branches, so if you know which branch is the more likely it can help. > > Obviously if you get it wrong it does not, but for the above cases it > seems to me that they are rather straightforward. > > It also provides some "this case is expected to be exceptional" semantics > to people reading the code. > > > I have a hard time believing that we're going to be much > > worse off if we just write the code normally. > > I think that your point applies to more general programming in postgres, > but this is not the context here. > > For low-level arithmetic code like this one, with tests and loops > containing very few hardware instructions, I think that helping compiler > optimizations is a good idea. Do you have any performance data to back that up? merlin
Re: Removing pg_pltemplate and creating "trustable" extensions
On Mon, Jan 6, 2020 at 1:27 PM Tom Lane wrote: > So, is that actually an objection to the current proposal, or just > an unrelated rant? Well, you brought up the topic of remaining bits in the context of the proposal, so I guess it's related. And I said pretty clearly that it wasn't necessarily an objection. But regarding your proposal: > After sleeping on it, I'm liking that idea; it's simple, and it > preserves the existing behavior that DB owners can install trusted PLs > without any extra permissions. Now, if we follow this up by marking > most of contrib as trusted, we'd be expanding that existing privilege. > But I think that's all right: I don't recall anybody ever complaining > that they wanted to prevent DB owners from installing trusted PLs, and > I do recall people wishing that it didn't take superuser to install > the other stuff. If somebody were to complain about this, what could they complain about? Potential complaints: 1. I'm the superuser and I don't want my DB owners to be able to install extensions other than trusted PLs. 2. Or I want to control which specific ones they can install. 3. I'm a non-superuser DB owner and I want to delegate permissions to install trusted extensions to some other user who is not a DB owner. All of those sound reasonably legitimate; against that, you can always argue that permissions should be more finely grained, and it's not always worth the implementation effort to make it possible. On #1, I tend to think that *most* people would be happy rather than sad about DB owners being able to install extensions; after all, evil extensions can be restricted by removing them from the disk (or marking them untrusted), and most people who set up a database are hoping it's going to get used for something. But somebody might not like it, especially if e.g. it turns out that one of our "trusted" extensions has a horrible security vulnerability. On #2, I can certainly imagine large providers having a view about which extensions they think are safe enough for users to install that differs from ours, and if that horrible security vulnerability materializes it sure would be nice to be able to easily disable access to just that one. #3 seems less likely to be an issue, but it's not unthinkable. "GRANT INSTALL ON mydb" seems like it would solve #1 and #3. You could grant a particular DB owner permission to install extensions, or not. If you have them that power WITH GRANT OPTION, then they could sub-delegate. It wouldn't do anything about #2; that would require some more complex scheme. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: weird libpq GSSAPI comment
Greetings, * Robbie Harwood (rharw...@redhat.com) wrote: > Alvaro Herrera writes: > > > How about this? > > > > * If GSSAPI is enabled and we can reach a credential cache, > > * set up a handle for it; if it's operating, just send a > > * GSS startup message, instead of the SSL negotiation and > > * regular startup message below. > > Due to the way postgres handled this historically, there are two ways > GSSAPI can be used: for connection encryption, and for authentication > only. We perform the same dance of sending a "request packet" for > GSSAPI encryption as we do for TLS encryption. So I'd like us to be > precise about which one we're talking about here (encryption). Alright, that's fair. > The GSSAPI idiom I should have used is "can acquire credentials" (i.e., > instead of "can reach a credential cache" in your proposal). Ok. > There's no such thing as a "GSS startup message". After negotiating > GSSAPI/TLS encryption (or failing to do so), we send the same things in > all cases, which includes negotiation of authentication mechanism if > any. (Negotiating GSSAPI for authentication after negotiating GSSAPI > for encryption will short-circuit rather than establishing a second > context, if I remember right.) Yes, you can see that around src/backend/libpq/auth.c:538 where we skip straight to pg_GSS_checkauth() if we already have encryption up and running, and if we don't then we go through pg_GSS_recvauth() (which will eventually call pg_GSS_checkauth() too). > I wonder if part of the confusion might be due to the synonyms we're > using here for "in use". Things seem to be "got running", "set up", > "operating", "negotiated", ... - maybe that's part of the barrier to > understanding? How about something like this? * If GSSAPI Encryption is enabled, then call pg_GSS_have_cred_cache() * which will return true if we can acquire credentials (and give us a * handle to use in conn->gcred), and then send a packet to the server * asking for GSSAPI Encryption (and skip past SSL negotiation and * regular startup below). Thanks, Stephen signature.asc Description: PGP signature
Re: weird libpq GSSAPI comment
Stephen Frost writes: >> Alvaro Herrera writes: > > How about something like this? > > * If GSSAPI Encryption is enabled, then call pg_GSS_have_cred_cache() > * which will return true if we can acquire credentials (and give us a > * handle to use in conn->gcred), and then send a packet to the server > * asking for GSSAPI Encryption (and skip past SSL negotiation and > * regular startup below). This looks correct to me (and uses plenty of parentheticals, so it feels in keeping with something I'd write) :) Thanks, --Robbie signature.asc Description: PGP signature
Re: TRUNCATE on foreign tables
Greetings, * Michael Paquier (mich...@paquier.xyz) wrote: > On Thu, Jan 02, 2020 at 09:46:44AM -0500, Stephen Frost wrote: > > I agree that the FDW callback should support multiple tables in the > > TRUNCATE, but I think it also should include CASCADE as an option and > > have that be passed on to the FDW to handle. > > As much as RESTRICT, ONLY and the INDENTITY clauses, no? Just think > about postgres_fdw. RESTRICT, yes. I don't know about ONLY being sensible as we don't really deal with inheritance and foreign tables very cleanly today, as I said up-thread, so I'm not sure if we really want to put in the effort that it'd require to figure out how to make ONLY make sense. The question about how to handle IDENTITY is a good one. I suppose we could just pass that down and let the FDW sort it out..? Thanks, Stephen signature.asc Description: PGP signature
Re: jsonb_set() strictness considered harmful to data
On Thu, Nov 28, 2019 at 2:15 PM Andrew Dunstan
wrote:
>
>
> On 11/27/19 9:35 PM, Michael Paquier wrote:
> > On Fri, Nov 15, 2019 at 09:45:59PM +0100, Pavel Stehule wrote:
> >> Maybe ERRCODE_NULL_VALUE_NOT_ALLOWED, and "NULL is not allowed",
> >> errdetail - a exception due setting "null_value_treatment" =>
> >> raise_exception
> >> and maybe some errhint - "Maybe you would to use Jsonb NULL -
> >> "null"::jsonb"
> >>
> >> I don't know, but in this case, the exception should be verbose. This is
> >> "rich" function with lot of functionality
> > @Andrew: This patch is waiting on input from you for a couple of days
> > now.
> >
>
>
Updated version including docco and better error message.
cheers
andrew
--
Andrew Dunstanhttps://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 57a1539506..7adb6a2d04 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -12203,6 +12203,9 @@ table2-mapping
jsonb_set
+
+ jsonb_set_lax
+
jsonb_insert
@@ -12517,6 +12520,26 @@ table2-mapping
[{"f1": 1, "f2": null, "f3": [2, 3, 4]}, 2]
+
+ jsonb_set_lax(target jsonb, path text[], new_value jsonb , create_missing boolean , null_value_treatment text)
+
+ jsonb
+
+If new_value is not null,
+behaves identically to jsonb_set. Otherwise behaves
+according to the value of null_value_treatment
+which must be one of 'raise_exception',
+'use_json_null', 'delete_key', or
+'return_target'. The default is
+'use_json_null'.
+
+ jsonb_set_lax('[{"f1":1,"f2":null},2,null,3]', '{0,f1}',null)
+ jsonb_set_lax('[{"f1":99,"f2":null},2]', '{0,f3}',null, true, 'return_target')
+
+ [{"f1":null,"f2":null},2,null,3]
+ [{"f1": 99, "f2": null}, 2]
+
+
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index 2fc3e3ff90..1cb2af1bcd 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -1237,6 +1237,15 @@ LANGUAGE INTERNAL
STRICT IMMUTABLE PARALLEL SAFE
AS 'jsonb_set';
+CREATE OR REPLACE FUNCTION
+ jsonb_set_lax(jsonb_in jsonb, path text[] , replacement jsonb,
+create_if_missing boolean DEFAULT true,
+null_value_treatment text DEFAULT 'use_json_null')
+RETURNS jsonb
+LANGUAGE INTERNAL
+CALLED ON NULL INPUT IMMUTABLE PARALLEL SAFE
+AS 'jsonb_set_lax';
+
CREATE OR REPLACE FUNCTION
parse_ident(str text, strict boolean DEFAULT true)
RETURNS text[]
diff --git a/src/backend/utils/adt/jsonfuncs.c b/src/backend/utils/adt/jsonfuncs.c
index ab5a24a858..b5fa4e7d18 100644
--- a/src/backend/utils/adt/jsonfuncs.c
+++ b/src/backend/utils/adt/jsonfuncs.c
@@ -4395,6 +4395,70 @@ jsonb_set(PG_FUNCTION_ARGS)
}
+/*
+ * SQL function jsonb_set_lax(jsonb, text[], jsonb, boolean, text)
+ */
+Datum
+jsonb_set_lax(PG_FUNCTION_ARGS)
+{
+ /* Jsonb *in = PG_GETARG_JSONB_P(0); */
+ /* ArrayType *path = PG_GETARG_ARRAYTYPE_P(1); */
+ /* Jsonb *newval = PG_GETARG_JSONB_P(2); */
+ /* bool create = PG_GETARG_BOOL(3); */
+ text *handle_null;
+ char *handle_val;
+
+ if (PG_ARGISNULL(0) || PG_ARGISNULL(1) || PG_ARGISNULL(3))
+ PG_RETURN_NULL();
+
+ /* could happen if they pass in an explicit NULL */
+ if (PG_ARGISNULL(4))
+ ereport(ERROR,
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("need delete_key, return_target, use_json_null, or raise_exception")));
+
+ /* if the new value isn't an SQL NULL just call jsonb_set */
+ if (! PG_ARGISNULL(2))
+ return jsonb_set(fcinfo);
+
+ handle_null = PG_GETARG_TEXT_P(4);
+ handle_val = text_to_cstring(handle_null);
+
+ if (strcmp(handle_val,"raise_exception") == 0)
+ {
+ ereport(ERROR,
+(errcode(ERRCODE_NULL_VALUE_NOT_ALLOWED),
+ errmsg("NULL is not allowed"),
+ errdetail("exception raised due to \"null_value_treatment := 'raise_exception'\""),
+ errhint("to avoid, either change the null_value_treatment argument or ensure that an SQL NULL is not used")));
+ }
+ else if (strcmp(handle_val, "use_json_null") == 0)
+ {
+ Datum newval;
+
+ newval = DirectFunctionCall1(jsonb_in, CStringGetDatum("null"));
+
+ fcinfo->args[2].value = newval;
+ fcinfo->args[2].isnull = false;
+ return jsonb_set(fcinfo);
+ }
+ else if (strcmp(handle_val, "delete_key") == 0)
+ {
+ return jsonb_delete_path(fcinfo);
+ }
+ else if (strcmp(handle_val, "return_target") == 0)
+ {
+ Jsonb *in = PG_GETARG_JSONB_P(0);
+ PG_RETURN_JSONB_P(in);
+ }
+ else
+ {
+ ereport(ERROR,
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("need delete_key, return_target, use_json_null, or raise_exception")));
+ }
+}
+
/*
* SQL function jsonb_delete_path(jsonb, text[])
*/
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_
Re: weird libpq GSSAPI comment
On 2020-Jan-06, Stephen Frost wrote: > > I wonder if part of the confusion might be due to the synonyms we're > > using here for "in use". Things seem to be "got running", "set up", > > "operating", "negotiated", ... - maybe that's part of the barrier to > > understanding? > > How about something like this? > > * If GSSAPI Encryption is enabled, then call pg_GSS_have_cred_cache() > * which will return true if we can acquire credentials (and give us a > * handle to use in conn->gcred), and then send a packet to the server > * asking for GSSAPI Encryption (and skip past SSL negotiation and > * regular startup below). WFM. (I'm not sure why you uppercase Encryption, though.) -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: weird libpq GSSAPI comment
Hello, On 2020-Jan-06, Robbie Harwood wrote: > This looks correct to me (and uses plenty of parentheticals, so it feels > in keeping with something I'd write) :) (You know, long ago I used to write with a lot of parenthicals (even nested ones). But I read somewhere that prose is more natural for normal people without them, so I mostly stopped using them.) Cheers, -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: [PATCH] fix a performance issue with multiple logical-decoding walsenders
Julien Rouhaud writes: > On Mon, Jan 6, 2020 at 7:57 PM Pierre Ducroquet wrote: >> My deepest apologies for the patch being broken, I messed up when >> transferring >> it between my computers after altering the comments. The verbatim one >> attached >> to this email applies with no issue on current HEAD. >> The patch regarding PostmasterIsAlive is completely pointless since v12 where >> the function was rewritten, and was included only to help reproduce the issue >> on older versions. Back-patching the walsender patch further than v12 would >> imply back-patching all the machinery introduced for PostmasterIsAlive >> (9f09529952) or another intrusive change there, a too big risk indeed. > +1, backpatch to v12 looks sensible. Pushed with some minor cosmetic fiddling. regards, tom lane
Re: weird libpq GSSAPI comment
Greetings, * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: > On 2020-Jan-06, Stephen Frost wrote: > > > > I wonder if part of the confusion might be due to the synonyms we're > > > using here for "in use". Things seem to be "got running", "set up", > > > "operating", "negotiated", ... - maybe that's part of the barrier to > > > understanding? > > > > How about something like this? > > > > * If GSSAPI Encryption is enabled, then call pg_GSS_have_cred_cache() > > * which will return true if we can acquire credentials (and give us a > > * handle to use in conn->gcred), and then send a packet to the server > > * asking for GSSAPI Encryption (and skip past SSL negotiation and > > * regular startup below). > > WFM. (I'm not sure why you uppercase Encryption, though.) Ok, great, attached is an actual patch which I'll push soon if there aren't any other comments. Thanks! Stephen From 49a57d5040c487c65cd9968504e978d11b4aefca Mon Sep 17 00:00:00 2001 From: Stephen Frost Date: Mon, 6 Jan 2020 16:49:02 -0500 Subject: [PATCH] Improve GSSAPI Encryption startup comment in libpq The original comment was a bit confusing, pointed out by Alvaro Herrera. Thread: https://postgr.es/m/20191224151520.GA16435%40alvherre.pgsql --- src/interfaces/libpq/fe-connect.c | 10 ++ 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c index 3bd30482ec..89b134665b 100644 --- a/src/interfaces/libpq/fe-connect.c +++ b/src/interfaces/libpq/fe-connect.c @@ -2800,10 +2800,12 @@ keep_going: /* We will come back to here until there is #ifdef ENABLE_GSS /* - * If GSSAPI is enabled and we have a credential cache, try to - * set it up before sending startup messages. If it's already - * operating, don't try SSL and instead just build the startup - * packet. + * If GSSAPI encryption is enabled, then call + * pg_GSS_have_cred_cache() which will return true if we can + * acquire credentials (and give us a handle to use in + * conn->gcred), and then send a packet to the server asking + * for GSSAPI Encryption (and skip past SSL negotiation and + * regular startup below). */ if (conn->try_gss && !conn->gctx) conn->try_gss = pg_GSS_have_cred_cache(&conn->gcred); -- 2.20.1 signature.asc Description: PGP signature
Re: psql FETCH_COUNT feature does not work with combined queries
On Fri, Jul 26, 2019 at 04:13:23PM +, Fabien COELHO wrote: FETCH_COUNT does not work with combined queries, and probably has never worked since 2006. For the record, this bug has already been reported & discussed by Daniel Vérité a few months back, see: https://www.postgresql.org/message-id/flat/a0a854b6-563c-4a11-bf1c-d6c6f924004d%40manitou-mail.org Given the points of view expressed on this thread, especially Tom's view against improving the lexer used by psql to known where query ends, it looks that my documentation patch is the way to go in the short term, and that if the "always show query results" patch is committed, it might simplify a bit solving this issue as the PQexec call is turned into PQsendQuery. Assuming there's no one willing to fix the behavior (and that seems unlikely given we're in the middle of a 2020-01 commitfest) it makes sense to at least document the behavior. That being said, I think the proposed patch may be a tad too brief. I don't think we need to describe the exact behavior, of course, but maybe it'd be good to mention that the behavior depends on the type of the last command etc. Also, I don't think "combined query" is a term used anywhere in the code/docs - I think the proper term is "multi-query string". regards -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Removing pg_pltemplate and creating "trustable" extensions
Robert Haas writes: > On Mon, Jan 6, 2020 at 1:27 PM Tom Lane wrote: >> After sleeping on it, I'm liking that idea; it's simple, and it >> preserves the existing behavior that DB owners can install trusted PLs >> without any extra permissions. Now, if we follow this up by marking >> most of contrib as trusted, we'd be expanding that existing privilege. >> But I think that's all right: I don't recall anybody ever complaining >> that they wanted to prevent DB owners from installing trusted PLs, and >> I do recall people wishing that it didn't take superuser to install >> the other stuff. > If somebody were to complain about this, what could they complain > about? Potential complaints: > 1. I'm the superuser and I don't want my DB owners to be able to > install extensions other than trusted PLs. > 2. Or I want to control which specific ones they can install. > 3. I'm a non-superuser DB owner and I want to delegate permissions to > install trusted extensions to some other user who is not a DB owner. Sure, but all of these seem to be desires for features that could be added later. As for #1, we could have that just by not taking the next step of marking anything but the PLs trusted (something that is going to happen anyway for v13, if this patch doesn't move faster). #2 is not a feature that exists now, either; actually, the patch *adds* it, to the extent that the superuser is willing to adjust extension control files. Likewise, #3 is not a feature that exists now. Also, the patch adds something that looks partly like #3, in that the superuser could grant pg_install_trusted_extension with admin option to database users who should be allowed to delegate it. Perhaps that's inadequate, but I don't see why we can't wait for complaints before trying to design something that satisfies hypothetical use cases. The facts that I'm worried about are that this is already the January 'fest, and if we don't want to ship v13 with python 2 as still the preferred python, we need to not only get this patch committed but do some less-than-trivial additional work (that hasn't even been started). So I'm getting very resistant to requests for more features in this patch. I think everything you're suggesting above could be tackled later, when and if there's actual field demand for it. > "GRANT INSTALL ON mydb" seems like it would solve #1 and #3. It's not apparent to me that that's better, and it seems possible that it's worse. The fact that a DB owner could grant that privilege to himself means that you might as well just have it on all the time. Like a table owner's DML rights, it would only be useful to prevent accidentally shooting yourself in the foot ... but who accidentally issues CREATE EXTENSION? And if they do (for an extension that deserves to be marked trusted) what harm is done really? Worst case is you drop it again. regards, tom lane
GSoC 2020
Greetings -hackers, Google Summer of Code is back for 2020! They have a similar set of requirements, expectations, and timeline as last year. Now is the time to be working to get together a set of projects we'd like to have GSoC students work on over the summer. Similar to last year, we need to have a good set of projects for students to choose from in advance of the deadline for mentoring organizations. The deadline for Mentoring organizations to apply is: February 5. The list of accepted organization will be published around February 20. Unsurprisingly, we'll need to have an Ideas page again, so I've gone ahead and created one (copying last year's): https://wiki.postgresql.org/wiki/GSoC_2020 Google discusses what makes a good "Ideas" list here: https://google.github.io/gsocguides/mentor/defining-a-project-ideas-list.html All the entries are marked with '2019' to indicate they were pulled from last year. If the project from last year is still relevant, please update it to be '2020' and make sure to update all of the information (in particular, make sure to list yourself as a mentor and remove the other mentors, as appropriate). New entries are certainly welcome and encouraged, just be sure to note them as '2020' when you add it. Projects from last year which were worked on but have significant follow-on work to be completed are absolutely welcome as well- simply update the description appropriately and mark it as being for '2020'. When we get closer to actually submitting our application, I'll clean out the '2019' entries that didn't get any updates. Also- if there are any projects that are no longer appropriate (maybe they were completed, for example and no longer need work), please feel free to remove them. I took a whack at that myself but it's entirely possible I missed some (and if I removed any that shouldn't have been- feel free to add them back by copying from the 2019 page). As a reminder, each idea on the page should be in the format that the other entries are in and should include: - Project title/one-line description - Brief, 2-5 sentence, description of the project (remember, these are 12-week projects) - Description of programming skills needed and estimation of the difficulty level - List of potential mentors - Expected Outcomes As with last year, please consider PostgreSQL to be an "Umbrella" project and that anything which would be considered "PostgreSQL Family" per the News/Announce policy [2] is likely to be acceptable as a PostgreSQL GSoC project. In other words, if you're a contributor or developer on WAL-G, barman, pgBackRest, the PostgreSQL website (pgweb), the PgEU/PgUS website code (pgeu-system), pgAdmin4, pgbouncer, pldebugger, the PG RPMs (pgrpms), the JDBC driver, the ODBC driver, or any of the many other PG Family projects, please feel free to add a project for consideration! If we get quite a few, we can organize the page further based on which project or maybe what skills are needed or similar. Let's have another great year of GSoC with PostgreSQL! Thanks! Stephen [1]: https://developers.google.com/open-source/gsoc/timeline [2]: https://www.postgresql.org/about/policies/news-and-events/ signature.asc Description: PGP signature
Re: pgsql: Add basic TAP tests for psql's tab-completion logic.
Christoph Berg writes: > Re: Tom Lane 2020-01-06 <3764.1578323...@sss.pgh.pa.us> >>> Does not work on Ubuntu bionic, xenial. (Others not tested.) >> Hmm ... do we care? The test output seems to show that xenial's >> 3.1-20150325-1ubuntu2 libedit is completely broken. Maybe there's >> a way to work around that, but it's not clear to me that that'd >> be a useful expenditure of time. You're not really going to be >> building PG13 for that release are you? > xenial (16.04) is a LTS release with support until 2021-04, and the > current plan was to support it. I now realize that's semi-close to the > 13 release date, but so far we have tried to really support all > PG-Distro combinations. I installed libedit_3.1-20150325.orig.tar.gz from source here, and it passes our current regression test and seems to behave just fine in light manual testing. (I did not apply any of the Debian-specific patches at [1], but they don't look like they'd explain much.) So I'm a bit at a loss as to what's going wrong for you. Is the test environment for Xenial the same as for the other branches? regards, tom lane [1] https://launchpad.net/ubuntu/+source/libedit/3.1-20150325-1ubuntu2
Re: Removing pg_pltemplate and creating "trustable" extensions
Greetings, * Tom Lane (t...@sss.pgh.pa.us) wrote: > Robert Haas writes: > > On Mon, Jan 6, 2020 at 1:27 PM Tom Lane wrote: > >> After sleeping on it, I'm liking that idea; it's simple, and it > >> preserves the existing behavior that DB owners can install trusted PLs > >> without any extra permissions. Now, if we follow this up by marking > >> most of contrib as trusted, we'd be expanding that existing privilege. > >> But I think that's all right: I don't recall anybody ever complaining > >> that they wanted to prevent DB owners from installing trusted PLs, and > >> I do recall people wishing that it didn't take superuser to install > >> the other stuff. > > > If somebody were to complain about this, what could they complain > > about? Potential complaints: > > > 1. I'm the superuser and I don't want my DB owners to be able to > > install extensions other than trusted PLs. I don't agree that this is actually a sensible use-case, so I'm not really sure why we're discussing solutions to make it work. It happens to be how things work because pg_pltemplate exists before we had extensions and we never went back and cleaned that up- but that's exactly what we're trying to do here, and adding in a nice feature at the same time. > > 2. Or I want to control which specific ones they can install. This is exactly what the 'trusted' bit is for, isn't it? If you think that we need something that's actually a permission matrix between roles and specific extensions, that's a whole different level, certainly, and I don't think anyone's asked for or contemplated such a need. I do like the idea of having a way to install more-or-less all extensions out on to the filesystem and then giving superusers an ability to decide which ones are 'trusted' and which ones are not, without having to hand-hack the control files. I don't particularly like using GUCs for that but I'm not sure what a better option looks like and I'm not completely convinced we really need this. If we really go down this route (without resorting to GUCs or something) then we'd need an additional catalog table that $someone is allowed to populate through some kind of SQL and a whole lot of extra work and I definitely don't think we need that right now. > > 3. I'm a non-superuser DB owner and I want to delegate permissions to > > install trusted extensions to some other user who is not a DB owner. This is a use-case that I do think exists (or, at least, I'm a superuser or a DB owner and I'd like to delegate that privilege to another user). > Sure, but all of these seem to be desires for features that could be > added later. We can't very well add a default role in one release and then decide we want to use the GRANT-privilege system in the next and remove it... > As for #1, we could have that just by not taking the > next step of marking anything but the PLs trusted (something that is > going to happen anyway for v13, if this patch doesn't move faster). Ugh. I find that to be a pretty horrible result. Yes, we could mark extensions later as 'trusted' but that'd be another year.. > #2 is not a feature that exists now, either; actually, the patch *adds* > it, to the extent that the superuser is willing to adjust extension > control files. Likewise, #3 is not a feature that exists now. Also, > the patch adds something that looks partly like #3, in that the > superuser could grant pg_install_trusted_extension with admin option > to database users who should be allowed to delegate it. Perhaps that's > inadequate, but I don't see why we can't wait for complaints before > trying to design something that satisfies hypothetical use cases. #3 from Robert's list certainly strikes me as a valid use-case and not just hypothetical. > The facts that I'm worried about are that this is already the January > 'fest, and if we don't want to ship v13 with python 2 as still the > preferred python, we need to not only get this patch committed but do > some less-than-trivial additional work (that hasn't even been started). > So I'm getting very resistant to requests for more features in this patch. > I think everything you're suggesting above could be tackled later, > when and if there's actual field demand for it. Perhaps I'm wrong, but I wouldn't think changing this from a default-role based approach over to a GRANT'able right using our existing GRANT system would be a lot of work. I agree that addressing some of the use-cases proposed above could be a great deal of work but, as I say above, I don't agree that we need to address all of them. > > "GRANT INSTALL ON mydb" seems like it would solve #1 and #3. > > It's not apparent to me that that's better, and it seems possible that > it's worse. The fact that a DB owner could grant that privilege to > himself means that you might as well just have it on all the time. I agree that the DB owner should have that right by default, just like they have any of the other DB-level rights that exist, just like
Re: Removing pg_pltemplate and creating "trustable" extensions
Stephen Frost writes: > Perhaps I'm wrong, but I wouldn't think changing this from a > default-role based approach over to a GRANT'able right using our > existing GRANT system would be a lot of work. Nobody has proposed a GRANT-based API that seems even close to acceptable from where I sit. A new privilege bit on databases is not it, at least not unless it works completely unlike any other privilege bit. It's giving control to the DB owners, not the superuser, and that seems like quite the wrong thing for this purpose. Or to put it another way: I think that the grantable role, which ultimately is handed out by the superuser, is the primary permissions API in this design. The fact that DB owners effectively have that same privilege is a wart for backwards-compatibility. If we were doing this from scratch, that wart wouldn't be there. What you're proposing is to make the wart the primary (indeed sole) permissions control mechanism for extension installation, and that just seems completely wrong. Superusers would have effectively *no* say in who gets to install trusted extensions, which is turning the whole thing on its head I think; it's certainly not responding to either of Robert's first two points. If we were willing to break backwards compatibility, what I'd prefer is to just have the grantable role, and to say that you have to grant that to DB owners if you want them to be able to install PLs. I'm not sure how loud the howls would be if we did that, but it'd be a lot cleaner than any of these other ideas. > I do *not* agree that this means we shouldn't have DB-level rights for > database owners and that we should just go hand-hack the system to have > explicit "is this the DB owner?" checks. The suggestion you're making > here seems to imply we should go hack up the CREATE SCHEMA check to have > it see if the user is the DB owner and then allow it, instead of doing > our normal privilege checks, and I don't think that makes any sense. Uh, what? Nothing in what I'm proposing goes anywhere near the permissions needed for CREATE SCHEMA. regards, tom lane
Re: Removing pg_pltemplate and creating "trustable" extensions
Greetings, * Tom Lane (t...@sss.pgh.pa.us) wrote: > Stephen Frost writes: > > Perhaps I'm wrong, but I wouldn't think changing this from a > > default-role based approach over to a GRANT'able right using our > > existing GRANT system would be a lot of work. > > Nobody has proposed a GRANT-based API that seems even close to > acceptable from where I sit. A new privilege bit on databases > is not it, at least not unless it works completely unlike > any other privilege bit. It's giving control to the DB owners, > not the superuser, and that seems like quite the wrong thing > for this purpose. I'm seriously confused by this. Maybe we need to step back for a moment because there are things that already exist today that I don't think we're really contemplating. The first is this- ANYONE can create an extension in the system today, if it's marked as superuser=false. If anything, it seems like that's probably too loose- certainly based on your contention that ONLY superusers should wield such a power and that letting anyone else do so is a right that a superuser must explicitly grant. > Or to put it another way: I think that the grantable role, which > ultimately is handed out by the superuser, is the primary permissions > API in this design. The fact that DB owners effectively have that > same privilege is a wart for backwards-compatibility. If we were > doing this from scratch, that wart wouldn't be there. What you're > proposing is to make the wart the primary (indeed sole) permissions > control mechanism for extension installation, and that just seems > completely wrong. Superusers would have effectively *no* say in > who gets to install trusted extensions, which is turning the whole > thing on its head I think; it's certainly not responding to either > of Robert's first two points. Superusers don't have any (direct) say in who gets to create schemas either, yet we don't seem to have a lot of people complaining about it. In fact, superusers don't have any say in who gets to create functions, or operators, or tables, or indexes, or EXTENSIONS, either. The fact is that DB owners can *already* create most objects, including extensions, in the DB system without the superuser being able to say anything about it. I really don't understand this hold-up when it comes to (trusted) extensions. Consider that, today, in many ways, PLs *are* the 'trusted' extensions that DB owners are already allowed to install. They're libraries of C functions that are trusted to do things right and therefore they can be allowed to be installed by DB owners. If we had a generic way to have a C library declare that it only exposes 'trusted' C functions, would we deny users the ability to create those functions in the database, when they can create functions in a variety of other trusted languages? Why would the fact that they're C functions, in that case, make them somehow special? That is, in fact, *exactly* what's already going on with pltemplate and trusted languages. Having trusted extensions is giving us exactly what pltemplate does, but in a generic way where any C library (or whatever else) can be declared as trusted, as defined by the extension framework around it, and therefore able to be installed by DB owners. Considering we haven't got any kind of check in the system today around extension creation, itself, this hardly seems like a large step to me- one could even argue that maybe we should just let ANYONE create them, but I'm not asking for that (in fact, I could probably be argued into agreeing to remove the ability for $anyone to create non-superuser extensions today, if we added this privilege...). > If we were willing to break backwards compatibility, what I'd prefer > is to just have the grantable role, and to say that you have to grant > that to DB owners if you want them to be able to install PLs. I'm > not sure how loud the howls would be if we did that, but it'd be a > lot cleaner than any of these other ideas. If we can't come to agreement regarding using a regular GRANT'able right, then I'd much rather break backwards compatibility than have such a hacked up wart like this special case you're talking about for PLs. > > I do *not* agree that this means we shouldn't have DB-level rights for > > database owners and that we should just go hand-hack the system to have > > explicit "is this the DB owner?" checks. The suggestion you're making > > here seems to imply we should go hack up the CREATE SCHEMA check to have > > it see if the user is the DB owner and then allow it, instead of doing > > our normal privilege checks, and I don't think that makes any sense. > > Uh, what? Nothing in what I'm proposing goes anywhere near the > permissions needed for CREATE SCHEMA. I understand that- you're talking about just having this 'wart' for CREATE EXTENSION and I don't agree with having the 'wart' at all. To start doing this for PLs would be completely inconsistent with the way the rest of the pr
Re: Assert failure due to "drop schema pg_temp_3 cascade" for temporary tables and \d+ is not showing any info after drooping temp table schema
On Mon, Jan 06, 2020 at 12:33:47PM -0500, Tom Lane wrote: > Robert Haas writes: >> I'm not arguing for a revert of 246a6c8. I think we should just change this: >> ... >> To look more like: > >> char *nspname = get_namespace_name(classForm->relnamespace); >> if (nspname != NULL) >>ereport(..."autovacuum: dropping orphan temp table \"%s.%s.%s\"...) >> else >>ereport(..."autovacuum: dropping orphan temp table with OID %u") > >> If we do that, then I think we can just revert >> a052f6cbb84e5630d50b68586cecc127e64be639 completely. > > +1 to both of those --- although I think we could still provide the > table name in the null-nspname case. Okay for the first one, printing the OID sounds like a good idea. Like Tom, I would prefer keeping the relation name with "(null)" for the schema name. Or even better, could we just print the OID all the time? What's preventing us from showing that information in the first place? And that still looks good to have when debugging issues IMO for orphaned entries. For the second one, I would really wish that we keep the restriction put in place by a052f6c until we actually figure out how to make the operation safe in the ways we want it to work because this puts the catalogs into an inconsistent state for any object type able to use a temporary schema, like functions, domains etc. for example able to use "pg_temp" as a synonym for the temp namespace name. And any connected user is able to do that. On top of that, except for tables, these could remain as orphaned entries after a crash, no? Allowing the operation only via allow_system_table_mods gives an exit path actually if you really wish to do so, which is fine by me as startup controls that, aka an administrator. In short, I don't think that it is sane to keep in place the property, visibly accidental (?) for any user to create inconsistent catalog entries using a static state in the session which is incorrect in namespace.c, except if we make DROP SCHEMA on a temporary schema have a behavior close to DISCARD TEMP. Again, for the owner of the session that's simple, no clear idea how to do that safely when the drop is done from another session not owning the temp schema. > Maybe we haven't. It's not clear that infrequent autovac crashes would > get reported to us, or that we'd successfully find the cause if they were. > > I think what you propose above is a back-patchable bug fix. Yeah, likely it is safer to fix the logs in the long run down to 9.4. -- Michael signature.asc Description: PGP signature
Re: Setting min/max TLS protocol in clientside libpq
On Mon, Jan 06, 2020 at 09:37:54AM -0500, Tom Lane wrote: > Magnus Hagander writes: >> Not having thought about it in much detail, but it's a fairly common >> scenario to have a much newer version of libpq (and the platform it's >> built on) than the server. E.g. a v12 libpq against a v9.6 postgres >> server is very common. For example, debian based systems will >> auto-upgrade your libpq, but not your server (for obvious reasons). >> And it's also quite common to upgrade platforms for the application >> much more frequently than the database server platform. > > Yeah, there's a reason why we expect pg_dump and psql to function with > ancient server versions. We shouldn't break this scenario with > careless rejiggering of libpq's connection defaults. Good points. Let's not do that then. -- Michael signature.asc Description: PGP signature
Re: RFC: seccomp-bpf support
Hi, This patch is currently in "needs review" state, but the last message is from August 29, and my understanding is that there have been a couple of objections / disagreements about the architecture, difficulties with producing the set of syscalls, and not providing any built-in policy. I don't think we're any closer to resolve those disagreements since August, so I think we should make some decision about this patch, instead of just moving it from one CF to the next one. The "needs review" status seems not reflecting the situation. Are there any plans to post a new version of the patch with a different design, or something like that? If not, I propose we mark it either as rejected or returned with feedback (and maybe get a new patch in the future). regards -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Greatest Common Divisor
Hello Merlin,
For low-level arithmetic code like this one, with tests and loops
containing very few hardware instructions, I think that helping compiler
optimizations is a good idea.
Do you have any performance data to back that up?
Yep.
A generic data is the woes about speculative execution related CVE (aka
Spectre) fixes and their impact on performance, which is in percents,
possibly tens of them, when the thing is more or less desactivated to
mitigate the security issue:
https://www.nextplatform.com/2018/03/16/how-spectre-and-meltdown-mitigation-hits-xeon-performance/
Some data about the __builtin_expect compiler hints:
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2016/p0479r0.html
Basically, they are talking about percents, up to tens in some cases,
which is consistent with the previous example.
As I said, helping the compiler is a good idea, and pg has been doing that
with the likely/unlikely macros for some time, there are over an hundred
of them, including in headers which get expanded ("logging.h", "float.h",
"simplehash.h", …), which is a good thing.
I do not see any good reason to stop doing that, especially in low-level
arithmetic functions.
Now, I do not have specific data about the performance impact on a gcd
implementation. Mileage may vary depending on hardware, compiler, options
and other overheads.
--
Fabien.
Re: Recognizing superuser in pg_hba.conf
Vik Fearing writes: > On 06/01/2020 17:03, Tom Lane wrote: >> So it's not clear to me whether we have any meeting of the minds >> on wanting this patch. In the meantime, though, the cfbot >> reports that the patch breaks the ssl tests. Why is that? > I have no idea. I cannot reproduce the failure locally. Hm, it blows up pretty thoroughly for me too, on a RHEL6 box. Are you sure you're running that test -- check-world doesn't do it? At least in the 001_ssltests test, the failures seem to all look like this in the TAP test's log file: psql: error: could not connect to server: could not initiate GSSAPI security context: Unspecified GSS failure. Minor code may provide more information could not initiate GSSAPI security context: Credentials cache file '/tmp/krb5cc_502' not found There are no matching entries in the postmaster log file, so this seems to be strictly a client-side failure. (Are we *really* putting security credentials in /tmp ???) regards, tom lane
Re: bitmaps and correlation
On Tue, Jan 7, 2020 at 1:29 AM Justin Pryzby wrote:
>
> Find attached cleaned up patch.
> For now, I updated the regress/expected/, but I think the test maybe has to be
> updated to do what it was written to do.
I have noticed that in "cost_index" we have used the indexCorrelation
for computing the run_cost, not the number of pages whereas in your
patch you have used it for computing the number of pages. Any reason
for the same?
cost_index
{
..
/*
* Now interpolate based on estimated index order correlation to get total
* disk I/O cost for main table accesses.
*/
csquared = indexCorrelation * indexCorrelation;
run_cost += max_IO_cost + csquared * (min_IO_cost - max_IO_cost);
}
Patch
- pages_fetched = (2.0 * T * tuples_fetched) / (2.0 * T + tuples_fetched);
+ pages_fetchedMAX = (2.0 * T * tuples_fetched) / (2.0 * T + tuples_fetched);
+
+ /* pages_fetchedMIN is for the perfectly correlated case (csquared=1) */
+ pages_fetchedMIN = ceil(indexSelectivity * (double) baserel->pages);
+
+ pages_fetched = pages_fetchedMAX +
indexCorrelation*indexCorrelation*(pages_fetchedMIN -
pages_fetchedMAX);
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
Re: bitmaps and correlation
On Tue, Jan 07, 2020 at 09:21:03AM +0530, Dilip Kumar wrote: > On Tue, Jan 7, 2020 at 1:29 AM Justin Pryzby wrote: > > > > Find attached cleaned up patch. > > For now, I updated the regress/expected/, but I think the test maybe has to > > be > > updated to do what it was written to do. > > I have noticed that in "cost_index" we have used the indexCorrelation > for computing the run_cost, not the number of pages whereas in your > patch you have used it for computing the number of pages. Any reason > for the same? As Jeff has pointed out, high correlation has two effects in cost_index(): 1) the number of pages read will be less; 2) the pages will be read more sequentially; cost_index reuses the pages_fetched variable, so (1) isn't particularly clear, but does essentially: /* max_IO_cost is for the perfectly uncorrelated case (csquared=0) */ pages_fetched(MIN) = index_pages_fetched(tuples_fetched, baserel->pages, (double) index->pages, root); max_IO_cost = pages_fetchedMIN * spc_random_page_cost; /* min_IO_cost is for the perfectly correlated case (csquared=1) */ pages_fetched(MAX) = ceil(indexSelectivity * (double) baserel->pages); min_IO_cost = (pages_fetchedMAX - 1) * spc_seq_page_cost; My patch 1) changes compute_bitmap_pages() to interpolate pages_fetched using the correlation; pages_fetchedMIN is new: > Patch > - pages_fetched = (2.0 * T * tuples_fetched) / (2.0 * T + tuples_fetched); > + pages_fetchedMAX = (2.0 * T * tuples_fetched) / (2.0 * T + tuples_fetched); > + > + /* pages_fetchedMIN is for the perfectly correlated case (csquared=1) */ > + pages_fetchedMIN = ceil(indexSelectivity * (double) baserel->pages); > + > + pages_fetched = pages_fetchedMAX + > indexCorrelation*indexCorrelation*(pages_fetchedMIN - pages_fetchedMAX); And, 2) also computes cost_per_page by interpolation between seq_page and random_page cost: + cost_per_page_corr = spc_random_page_cost - + (spc_random_page_cost - spc_seq_page_cost) + * (1-correlation*correlation); Thanks for looking. I'll update the name of pages_fetchedMIN/MAX in my patch for consistency with cost_index. Justin
pg_repack failure
Hello, When I tried to repack my bloated table an error occurred: FATAL: terminating connection due to idle-in-transaction timeout ERROR: query failed: SSL connection has been closed unexpectedly DETAIL: query was: SAVEPOINT repack_sp1 and this error is occurring in large tables only, and current table size which is running about 700GB /pg_repack --version pg_repack 1.4.3 DB version: PostgreSQL 9.6.11 on x86_64-pc-linux-gnu, compiled by gcc (GCC) 4.9.3, 64-bit Appreciate the assistance. FATAL: terminating connection due to idle-in-transaction timeout · Issue #222 · reorg/pg_repack | | | | | | | | | | | FATAL: terminating connection due to idle-in-transaction timeout · Issu... When I tried to repack my bloated table an error occurred: FATAL: terminating connection due to idle-in-transact... | | | Regards,Nagaraj
Re: pgsql: Add basic TAP tests for psql's tab-completion logic.
On Tue, Jan 7, 2020 at 12:43 AM Christoph Berg wrote: > > Re: Tom Lane 2020-01-06 <3764.1578323...@sss.pgh.pa.us> > > > Does not work on Ubuntu bionic, xenial. (Others not tested.) > > > > Hmm ... do we care? The test output seems to show that xenial's > > 3.1-20150325-1ubuntu2 libedit is completely broken. Maybe there's > > a way to work around that, but it's not clear to me that that'd > > be a useful expenditure of time. You're not really going to be > > building PG13 for that release are you? > > xenial (16.04) is a LTS release with support until 2021-04, and the > current plan was to support it. I now realize that's semi-close to the > 13 release date, but so far we have tried to really support all > PG-Distro combinations. > > I could probably arrange for that test to be disabled when building > for xenial, but it'd be nice if there were a configure switch or > environment variable for it so we don't have to invent it. > > > > Ubuntu bionic fails elsewhere [ apparently in 006_logical_decoding.pl ] > > > > Hmm. Not related to this thread then. But if that's reproducible, > > It has been failing with the same output since Jan 2, 2020, noon. > > > somebody should have a look. Maybe related to > > > > https://www.postgresql.org/message-id/CAA4eK1LMDx6vK8Kdw8WUeW1MjToN2xVffL2kvtHvZg17%3DY6QQg%40mail.gmail.com > > The only git change in that build was d207038053837 "Fix running out > of file descriptors for spill files." > Thanks, for reporting. Is it possible to get a call stack as we are not able to reproduce this failure? Also, if you don't mind, let's discuss this on the thread provided by Tom. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Re: pg_repack failure
On Tue, Jan 07, 2020 at 06:15:09AM +, Nagaraj Raj wrote: > and this error is occurring in large tables only, and current table > size which is running about 700GB > > /pg_repack --version > pg_repack 1.4.3 > > DB version: PostgreSQL 9.6.11 on x86_64-pc-linux-gnu, compiled by gcc (GCC) > 4.9.3, 64-bit I think that you had better report that directly to the maintainers of the tool here: https://github.com/reorg/pg_repack/ -- Michael signature.asc Description: PGP signature
Re: Fetching timeline during recovery
At Fri, 3 Jan 2020 16:11:38 +0100, Jehan-Guillaume de Rorthais wrote in > Hi, > > On Mon, 23 Dec 2019 15:38:16 +0100 > Jehan-Guillaume de Rorthais wrote: > [...] > > My idea would be to return a row from pg_stat_get_wal_receiver() as soon as > > a wal receiver has been replicating during the uptime of the standby, no > > matter if there's one currently working or not. If no wal receiver is > > active, > > the "pid" field would be NULL and the "status" would reports eg. "inactive". > > All other fields would report their last known value as they are kept in > > shared memory WalRcv struct. > > Please, find in attachment a patch implementing the above proposal. At Mon, 23 Dec 2019 15:38:16 +0100, Jehan-Guillaume de Rorthais wrote in > 1. we could decide to remove this filter to expose the data even when no wal > receiver is active. It's the same behavior than pg_stat_subscription view. > It could introduce regression from tools point of view, but adds some > useful information. I would vote 0 for it. A subscription exists since it is defined and regardless whether it is active or not. It is strange that we see a line in the view if replication is not configured. But it is reasonable to show if it is configured. We could do that by checking PrimaryConnInfo. (I would vote +0.5 for it). > 2. we could extend it with new replayed lsn/tli fields. I would vote +1 for > it. +1. As of now a walsender lives for just one timeline, because it ends for disconnection from walsender when the master moves to a new timeline. That being said, we already have the columns for TLI for both starting and received-up-to LSN so we would need it also for replayed LSN for a consistent looking. The function is going to show "streaming" but conninfo is not shown until connection establishes. That state is currently hidden by the PID filtering of the view. We might need to keep the WALRCV_STARTING state until connection establishes. sender_host and sender_port have bogus values until connection is actually established when conninfo is changed. They as well as conninfo should be hidden until connection is established, too, I think. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Re: TRUNCATE on foreign tables
On Mon, Jan 06, 2020 at 04:32:39PM -0500, Stephen Frost wrote: > RESTRICT, yes. I don't know about ONLY being sensible as we don't > really deal with inheritance and foreign tables very cleanly today, as I > said up-thread, so I'm not sure if we really want to put in the effort > that it'd require to figure out how to make ONLY make sense. True enough. > The question about how to handle IDENTITY is a good one. I suppose > we could just pass that down and let the FDW sort it out..? Looking at the code, ExecuteTruncateGuts() passes down restart_seqs, so it sounds sensible to me to just pass down that to the FDW callback and the callback decide what to do. -- Michael signature.asc Description: PGP signature
DROP OWNED CASCADE vs Temp tables
I have a test where a user creates a temp table and then disconnect, concurrently we try to do DROP OWNED BY CASCADE on the same user. Seems this causes race condition between temptable deletion during disconnection (@RemoveTempRelations(myTempNamespace)) and DROP OWNED BY CASCADE operation which will try to remove same temp table when they find them as part of pg_shdepend. Which will result in internal error cache lookup failed as below. DROP OWNED BY test_role CASCADE; 2020-01-07 12:35:06.524 IST [26064] ERROR: cache lookup failed for relation 41019 2020-01-07 12:35:06.524 IST [26064] STATEMENT: DROP OWNED BY test_role CASCADE; reproduce.sql:8: ERROR: cache lookup failed for relation 41019 TEST = create database test_db; create user test_superuser superuser; \c test_db test_superuser CREATE ROLE test_role nosuperuser login password 'test_pwd' ; \c test_db test_role CREATE TEMPORARY TABLE tmp_table(col1 int); \c test_db test_superuser DROP OWNED BY test_role CASCADE; -- Thanks and Regards Mithun Chicklore Yogendra EnterpriseDB: http://www.enterprisedb.com
