Re: OpenSSL randomness seeding
On Sat, Aug 01, 2020 at 11:48:23PM -0700, Noah Misch wrote: > On Thu, Jul 30, 2020 at 11:42:16PM +0200, Daniel Gustafsson wrote: >> Somewhat on topic though, 1.1.1 adds a RAND_priv_bytes function for random >> numbers that are supposed to be private and extra protected via it's own >> DRBG. >> Maybe we should use that for SCRAM salts etc in case we detect 1.1.1? > > Maybe. Would you have a separate pg_private_random() function, or just use > RAND_priv_bytes() for pg_strong_random()? No pg_strong_random() caller is > clearly disinterested in privacy; gen_random_uuid() may come closest. FWIW, I am not sure that we need extra level of complexity when it comes to random number generation, so having only one API to rule them all sounds sensible to me, particularly if we know that the API used has more private protections. -- Michael signature.asc Description: PGP signature
Re: problem with RETURNING and update row movement
Yet another thing I noticed is that the patch incorrectly produces
values for the tableoid columns specified in the RETURNING list, like
this:
+UPDATE range_parted r set a = 'c' FROM (VALUES ('a', 1), ('a', 10),
('b', 12)) s(x, y) WHERE s.x = r.a AND s.y = r.b RETURNING
tableoid::regclass, *;
+tableoid| a | b | c | d | e | x | y
++---++-+---+---+---+
+ part_a_1_a_10 | c | 1 | 1 | 1 | in trigfunc() | a | 1
+ part_a_10_a_20 | c | 10 | 200 | 1 | in trigfunc() | a | 10
+ part_c_1_100 | c | 12 | 96 | 1 | in trigfunc() | b | 12
+(3 rows)
The source partitions are shown as tableoid, but the destination
partition (ie, part_c_1_c_20) should be shown. To fix this, I
modified the patch further so that 1) we override tts_tableOid of the
original slot with the OID of the destination partition before calling
ExecProcessReturning() if needed, and 2) in ExecProcessReturning(), we
only initialize ecxt_scantuple's tts_tableOid when needed, which would
save cycles a bit for non-foreign-table-direct-modification cases.
Attached is a new version of the patch.
Best regards,
Etsuro Fujita
v4-0001-Fix-a-bug-with-RETURNING-when-UPDATE-moves-tuple.patch
Description: Binary data
Re: Index Skip Scan (new UniqueKeys)
Hi David:
Thanks for looking into this.
On Fri, Jul 31, 2020 at 11:07 AM David Rowley wrote:
> On Mon, 13 Jul 2020 at 10:18, Floris Van Nee
> wrote:
> > One question about the unique keys - probably for Andy or David: I've
> looked in the archives to find arguments for/against using Expr nodes or
> EquivalenceClasses in the Unique Keys patch. However, I couldn't really
> find a clear answer about why the current patch uses Expr rather than
> EquivalenceClasses. At some point David mentioned "that probably Expr nodes
> were needed rather than EquivalenceClasses", but it's not really clear to
> me why. What were the thoughts behind this?
>
> I'm still not quite sure on this either way. I did think
> EquivalenceClasses were more suitable before I wrote the POC patch for
> unique keys. But after that, I had in mind that Exprs might be
> better. The reason I thought this was due to the fact that the
> DISTINCT clause list is a bunch of Exprs and if the UniqueKeys were
> EquivalenceClasses then checking to see if the DISTINCT can be skipped
> turned into something more complex that required looking through lists
> of ec_members rather than just checking if the uniquekey exprs were a
> subset of the DISTINCT clause.
>
> Thinking about it a bit harder, if we did use Exprs then it would mean
> it a case like the following wouldn't work for Andy's DISTINCT no-op
> stuff.
>
> CREATE TABLE xy (x int primary key, y int not null);
>
> SELECT DISTINCT y FROM xy WHERE x=y;
>
> whereas if we use EquivalenceClasses then we'll find that we have an
> EC with x,y in it and can skip the DISTINCT since we have a UniqueKey
> containing that EquivalenceClass.
> Also, looking at what Andy wrote to make a case like the following
> work in his populate_baserel_uniquekeys() function in the 0002 patch:
>
> CREATE TABLE ab (a int, b int, primary key(a,b));
> SELECT DISTINCT a FROM ab WHERE b = 1;
>
> it's a bit uninspiring. Really what we want here when checking if we
> can skip doing the DISTINCT is a UniqueKey set using
> EquivalenceClasses as we can just insist that any unmatched UniqueKey
> items have an ec_is_const == true. However, that means we have to loop
> through the ec_members of the EquivalenceClasses in the uniquekeys
> during the DISTINCT check. That's particularly bad when you consider
> that in a partitioned table case there might be an ec_member for each
> child partition and there could be 1000s of child partitions and
> following those ec_members chains is going to be too slow.
>
> My current thoughts are that we should be using EquivalenceClasses but
> we should first add some infrastructure to make them perform better.
> My current thoughts are that we do something like what I mentioned in
> [1] or something more like what Andres mentions in [2]. After that,
> we could either make EquivalenceClass.ec_members a hash table or
> binary search tree. Or even perhaps just have a single hash table/BST
> for all EquivalenceClasses that allows very fast lookups from {Expr}
> -> {EquivalenceClass}. I think an Expr can only belong in a single
> non-merged EquivalenceClass. So when we do merging of
> EquivalenceClasses we could just repoint that data structure to point
> to the new EquivalenceClass. We'd never point to ones that have
> ec_merged != NULL. This would also allow us to fix the poor
> performance in regards to get_eclass_for_sort_expr() for partitioned
> tables.
>
> So, it seems the patch dependency chain for skip scans just got a bit
> longer :-(
>
>
I admit that EquivalenceClasses has a better expressive power. There are
2 more
cases we can handle better with EquivalenceClasses. SELECT DISTINCT a, b,
c
FROM t WHERE a = b; Currently the UniqueKey is (a, b, c), but it is better
be (a, c)
and (b, c). The other case happens similarly in group by case.
After realizing this, I am still hesitant to do that, due to the
complexity. If we do that,
we may have to maintain a EquivalenceClasses in one more place or make the
existing
EquivalenceClasses List longer, for example: SELECT pk FROM t; The
current
infrastructure doesn't create any EquivalenceClasses for pk. So we have to
create
a new one in this case and reuse some existing ones in other cases.
Finally since the
EquivalenceClasses is not so straight to upper user, we have to depend on
the
infrastructure change to look up an EquivalenceClasses quickly from an
Expr.
I rethink more about the case you provide above, IIUC, there is such issue
for joinrel.
then we can just add a EC checking for populate_baserel_uniquekeys. As for
the
DISTINCT/GROUP BY case, we should build the UniqueKeys from
root->distinct_pathkeys
and root->group_pathkeys where the EquivalenceClasses are already there.
I am still not insisting on either Expr or EquivalenceClasses right now,
if we need to
change it to EquivalenceClasses, I'd see if we need to have more places to
take
care before doing that.
--
Best Regards
Andy Fan
Re: [HACKERS] [PATCH] Generic type subscripting
so 1. 8. 2020 v 16:30 odesílatel Dmitry Dolgov <9erthali...@gmail.com> napsal: > > On Fri, Jul 31, 2020 at 03:35:22PM -0400, Tom Lane wrote: > > > > I started to look through this again, and really found myself wondering > > why we're going to all this work to invent what are fundamentally pretty > > bogus "features". The thing that particularly sticks in my craw is the > > 0005 patch, which tries to interpret a subscript of a JSON value as > either > > integer or text depending on, seemingly, the phase of the moon. I don't > > think that will work. For example, with existing arrays you can do > > something like arraycol['42'] and the unknown-type literal is properly > > cast to an integer. The corresponding situation with a JSON subscript > > would have no principled resolution. > > > > It doesn't help any that both coercion alternatives are attempted at > > COERCION_ASSIGNMENT level, which makes it noticeably more likely that > > they'll both succeed. But using ASSIGNMENT level is quite inappropriate > > in any context where it's not 100% certain what the intended type is. > > > > The proposed commit message for 0005 claims that this is somehow > improving > > our standards compliance, but I see nothing in the SQL spec suggesting > > that you can subscript a JSON value at all within the SQL language, so > > I think that claim is just false. > > It's due to my lack of writing skills. As far as I can remember the > discussion was about JSON path part of the standard, where one allowed > to use float indexes with implementation-defined rounding or truncation. > In this patch series I'm trying to think of JSON subscript as an > equivalent for JSON path, hence this misleading description. Having said > that, I guess the main motivation behind 0005 is performance > improvements. Hopefully Nikita can provide more insights. Overall back > when 0005 patch was suggested its implementation looked reasonable for > me, but I'll review it again. > > > Maybe this could be salvaged by flushing 0005 in its current form and > > having the jsonb subscript executor do something like "if the current > > value-to-be-subscripted is a JSON array, then try to convert the textual > > subscript value to an integer". Not sure about what the error handling > > rules ought to be like, though. > > I'm fine with the idea of separating 0005 patch and potentially prusuing > it as an independent item. Just need to rebase 0006, since Pavel > mentioned that it's a reasonable change he would like to see in the > final result. > +1 Pavel
Re: psql - improve test coverage from 41% to 88%
On 8/1/20 5:27 PM, Tom Lane wrote: > Daniel Gustafsson writes: >> On 1 Aug 2020, at 09:06, Fabien COELHO wrote: >>> AFAICR the feedback is that the Expect perl module is not welcome, which >>> seems to suggest that it would have to be re-implemented somehow. This is >>> not my dev philosophy, I won't do that, so I'm sorry to say that psql >>> coverage will remain pretty abysmal. >> Re-reading this thread, I see no complaints about introducing a dependency on >> Expect. The feedback returned in this case is that the patch hasn't applied >> since March, and that the patch is more than welcome to be re-entered in the >> next CF once it does. > Personally, I'd object to introducing a hard dependency on Expect, as > there are no doubt a lot of developers and buildfarm members who don't > have that installed. But I see no reason we couldn't skip some tests > if it's lacking, as we're already doing with IO::Pty in > 010_tab_completion.pl. > > That does raise the question of whether Expect makes things enough > easier than raw IO::Pty that it's worth adding that dependency (and > hence foregoing the tests on some machines). But I'm prepared to be > convinced on that point. > > +1. Also note that the Windows animals don't and probably will never support Expect, since Windows doesn't have PTYs. Expect.pm is in fact a pure perl module that sits on top of IO::Pty, which in turn sits on top of IO::Tty. So if you have those Expect.pm probably isn't a huge stretch. But let's not add a dependency if we can avoid it. And if we do add one it will need to be a soft one like the case you mentioned. cheers andrew -- Andrew Dunstanhttps://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: [PATCH] Btree BackwardScan race condition on Standby during VACUUM
Hello, Peter. > Attached is a revised version of your patch Thanks for your work, the patch is looking better now. Michail.
Re: LDAP check flapping on crake due to race
Noah Misch writes:
> On Sun, Aug 02, 2020 at 05:29:57PM +1200, Thomas Munro wrote:
>> There are one or two failures per month on crake. It looks like when
>> authentication is rejected, as expected in the tests, the psql process
>> is exiting, but there is a race where the Perl script still wants to
>> write a dummy query to its stdin (?), so you get:
>> psql: FATAL: LDAP authentication failed for user "test1"
>> ack Broken pipe: write( 13, 'SELECT 1' ) at
>> /usr/share/perl5/vendor_perl/IPC/Run/IO.pm line 549.
> Do you suppose a fix like e12a472 would cover this? ("psql <&-" fails with
> status 1 after successful authentication, and authentication failure gives
> status 2.)
AFAICT the failure is happening down inside PostgresNode::psql's call
of IPC::Run::run, so we don't really have the option to adjust things
in exactly that way. (And messing with sub psql for the benefit of
this one caller seems pretty risky anyway.)
I'm inclined to suggest that the LDAP test's test_access could use
an empty stdin and pass "-c 'SELECT 1'" as a command line option
instead. (Maybe that's exactly what you meant, but I'm not sure.)
I've not been able to duplicate this locally, so I have no idea if
that'd really fix it.
regards, tom lane
Removing <@ from contrib/intarray's GiST opclasses
As previously discussed at [1], contrib/intarray's GiST opclasses
do not index empty arrays in a useful way, meaning that
"indexedcol <@ something" has to do a full-index search to ensure
that it finds empty arrays, which such a query should always find.
We'd be better off to not consider <@ indexable at all by these
opclasses, but removing it has been problematic because of
dependencies [2]. Now that commit 9f9682783 is in, the dependency
problem is fixed, so here are a couple of patches to remove the
operator's opclass membership.
Patch 0001 is a minimal patch to just drop the opclass membership.
We could do that and stop there, but if we do, <@ searches will
continue to be slow until people think to update their extensions
(which pg_upgrade does nothing to encourage). Alternatively,
we could replace the now-dead support code with something that
throws an error telling people to update the extension, as in 0002.
I'm honestly not sure whether 0002 is a good idea or not. Thoughts?
regards, tom lane
[1] https://www.postgresql.org/message-id/flat/458.1565114141%40sss.pgh.pa.us
[2] https://www.postgresql.org/message-id/flat/4578.1565195302%40sss.pgh.pa.us
diff --git a/contrib/intarray/Makefile b/contrib/intarray/Makefile
index b68959ebd6..01faa36b10 100644
--- a/contrib/intarray/Makefile
+++ b/contrib/intarray/Makefile
@@ -12,7 +12,8 @@ OBJS = \
_intbig_gist.o
EXTENSION = intarray
-DATA = intarray--1.2--1.3.sql intarray--1.2.sql intarray--1.1--1.2.sql \
+DATA = intarray--1.3--1.4.sql intarray--1.2--1.3.sql \
+ intarray--1.2.sql intarray--1.1--1.2.sql \
intarray--1.0--1.1.sql
PGFILEDESC = "intarray - functions and operators for arrays of integers"
diff --git a/contrib/intarray/intarray--1.3--1.4.sql b/contrib/intarray/intarray--1.3--1.4.sql
new file mode 100644
index 00..3fbebb5417
--- /dev/null
+++ b/contrib/intarray/intarray--1.3--1.4.sql
@@ -0,0 +1,21 @@
+/* contrib/intarray/intarray--1.3--1.4.sql */
+
+-- complain if script is sourced in psql, rather than via ALTER EXTENSION
+\echo Use "ALTER EXTENSION intarray UPDATE TO '1.4'" to load this file. \quit
+
+-- Remove <@ from the GiST opclasses, as it's not usefully indexable
+-- due to mishandling of empty arrays. (It's OK in GIN.)
+
+ALTER OPERATOR FAMILY gist__int_ops USING gist
+DROP OPERATOR 8 (_int4, _int4);
+
+ALTER OPERATOR FAMILY gist__intbig_ops USING gist
+DROP OPERATOR 8 (_int4, _int4);
+
+-- Likewise for the old spelling ~.
+
+ALTER OPERATOR FAMILY gist__int_ops USING gist
+DROP OPERATOR 14 (_int4, _int4);
+
+ALTER OPERATOR FAMILY gist__intbig_ops USING gist
+DROP OPERATOR 14 (_int4, _int4);
diff --git a/contrib/intarray/intarray.control b/contrib/intarray/intarray.control
index db7746b6c7..bbc837c573 100644
--- a/contrib/intarray/intarray.control
+++ b/contrib/intarray/intarray.control
@@ -1,6 +1,6 @@
# intarray extension
comment = 'functions, operators, and index support for 1-D arrays of integers'
-default_version = '1.3'
+default_version = '1.4'
module_pathname = '$libdir/_int'
relocatable = true
trusted = true
diff --git a/contrib/intarray/_int_gist.c b/contrib/intarray/_int_gist.c
index fb05b06af9..fd05247bbf 100644
--- a/contrib/intarray/_int_gist.c
+++ b/contrib/intarray/_int_gist.c
@@ -93,17 +93,19 @@ g_int_consistent(PG_FUNCTION_ARGS)
break;
case RTContainedByStrategyNumber:
case RTOldContainedByStrategyNumber:
- if (GIST_LEAF(entry))
-retval = inner_int_contains(query,
- (ArrayType *) DatumGetPointer(entry->key));
- else
- {
-/*
- * Unfortunately, because empty arrays could be anywhere in
- * the index, we must search the whole tree.
- */
-retval = true;
- }
+
+ /*
+ * Unfortunately, because empty arrays could be anywhere in the
+ * index, implementing this would require searching the whole
+ * tree. As of intarray 1.4, <@ is no longer part of the opclass.
+ * If we get here anyway, an obsolete opclass definition must
+ * still be installed.
+ */
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("<@ operator is no longer supported by gist__int_ops"),
+ errdetail("Please update to intarray 1.4 or later.")));
+ retval = false; /* keep compiler quiet */
break;
default:
retval = false;
diff --git a/contrib/intarray/_intbig_gist.c b/contrib/intarray/_intbig_gist.c
index 67c44e99a9..5fe110e9ce 100644
--- a/contrib/intarray/_intbig_gist.c
+++ b/contrib/intarray/_intbig_gist.c
@@ -533,39 +533,19 @@ g_intbig_consistent(PG_FUNCTION_ARGS)
break;
case RTContainedByStrategyNumber:
case RTOldContainedByStrategyNumber:
- if (GIST_LEAF(entry))
- {
-int i,
- num = ARRNELEMS(query);
-int32 *ptr = ARRPTR(query);
-BITVECP dq = palloc0(siglen),
- de;
-while (num--)
-{
- HASH(dq, *ptr, siglen);
- ptr++;
-}
-
-de = GETSIGN((GISTTYPE *) DatumGetPointer(entry->key));
-retval = true;
-LOOPBYTE(siglen)
-{
-
FailedAssertion("pd_idx == pinfo->nparts", File: "execPartition.c", Line: 1689)
Core was generated by `postgres: telsasoft ts [local] BIND
'.
(gdb) bt
#0 0x7f0951303387 in raise () from /lib64/libc.so.6
#1 0x7f0951304a78 in abort () from /lib64/libc.so.6
#2 0x00921005 in ExceptionalCondition
(conditionName=conditionName@entry=0xa5db3d "pd_idx == pinfo->nparts",
errorType=errorType@entry=0x977389 "FailedAssertion",
fileName=fileName@entry=0xa5da88 "execPartition.c",
lineNumber=lineNumber@entry=1689) at assert.c:67
#3 0x00672806 in ExecCreatePartitionPruneState
(planstate=planstate@entry=0x908f6d8, partitionpruneinfo=) at
execPartition.c:1689
#4 0x0068444a in ExecInitAppend (node=node@entry=0x7036b90,
estate=estate@entry=0x11563f0, eflags=eflags@entry=16) at nodeAppend.c:132
#5 0x006731fd in ExecInitNode (node=0x7036b90,
estate=estate@entry=0x11563f0, eflags=eflags@entry=16) at execProcnode.c:179
#6 0x0069d03a in ExecInitResult (node=node@entry=0x70363d8,
estate=estate@entry=0x11563f0, eflags=eflags@entry=16) at nodeResult.c:210
#7 0x0067323c in ExecInitNode (node=0x70363d8,
estate=estate@entry=0x11563f0, eflags=eflags@entry=16) at execProcnode.c:164
#8 0x0069e834 in ExecInitSort (node=node@entry=0x7035ca8,
estate=estate@entry=0x11563f0, eflags=eflags@entry=16) at nodeSort.c:210
#9 0x00672ff0 in ExecInitNode (node=0x7035ca8,
estate=estate@entry=0x11563f0, eflags=eflags@entry=16) at execProcnode.c:313
#10 0x006812e8 in ExecInitAgg (node=node@entry=0x68311d0,
estate=estate@entry=0x11563f0, eflags=eflags@entry=16) at nodeAgg.c:3292
#11 0x00672fb1 in ExecInitNode (node=0x68311d0,
estate=estate@entry=0x11563f0, eflags=eflags@entry=16) at execProcnode.c:328
#12 0x0068925a in ExecInitGatherMerge (node=node@entry=0x6830998,
estate=estate@entry=0x11563f0, eflags=eflags@entry=16) at nodeGatherMerge.c:110
#13 0x00672f33 in ExecInitNode (node=0x6830998,
estate=estate@entry=0x11563f0, eflags=eflags@entry=16) at execProcnode.c:348
#14 0x006812e8 in ExecInitAgg (node=node@entry=0x682eda8,
estate=estate@entry=0x11563f0, eflags=eflags@entry=16) at nodeAgg.c:3292
#15 0x00672fb1 in ExecInitNode (node=node@entry=0x682eda8,
estate=estate@entry=0x11563f0, eflags=eflags@entry=16) at execProcnode.c:328
#16 0x0066c8e6 in InitPlan (eflags=16, queryDesc=) at
execMain.c:1020
#17 standard_ExecutorStart (queryDesc=, eflags=16) at
execMain.c:266
#18 0x7f0944ca83b5 in pgss_ExecutorStart (queryDesc=0x1239b08,
eflags=) at pg_stat_statements.c:1007
#19 0x7f09117e4891 in explain_ExecutorStart (queryDesc=0x1239b08,
eflags=) at auto_explain.c:301
#20 0x007f9983 in PortalStart (portal=0xeff810, params=0xfacc98,
eflags=0, snapshot=0x0) at pquery.c:505
#21 0x007f7370 in PostgresMain (argc=,
argv=argv@entry=0xeb8500, dbname=0xeb84e0 "ts", username=) at
postgres.c:1987
#22 0x0048916e in BackendRun (port=, port=) at postmaster.c:4523
#23 BackendStartup (port=0xeb1000) at postmaster.c:4215
#24 ServerLoop () at postmaster.c:1727
#25 0x0076ec85 in PostmasterMain (argc=argc@entry=13,
argv=argv@entry=0xe859b0) at postmaster.c:1400
#26 0x0048a82d in main (argc=13, argv=0xe859b0) at main.c:210
#3 0x00672806 in ExecCreatePartitionPruneState
(planstate=planstate@entry=0x908f6d8, partitionpruneinfo=) at
execPartition.c:1689
pd_idx =
pp_idx =
pprune = 0x908f910
partdesc = 0x91937f8
pinfo = 0x7d6ee78
partrel =
partkey = 0xfbba28
lc2__state = {l = 0x7d6ee20, i = 0}
partrelpruneinfos = 0x7d6ee20
lc2 =
npartrelpruneinfos =
prunedata = 0x908f908
j = 0
lc__state = {l = 0x7d6edc8, i = 0}
estate = 0x11563f0
prunestate = 0x908f8b0
n_part_hierarchies =
lc =
i = 0
(gdb) p *pinfo
$2 = {type = T_PartitionedRelPruneInfo, rtindex = 7, present_parts = 0x7d6ef10,
nparts = 414, subplan_map = 0x7d6ef68, subpart_map = 0x7d6f780, relid_map =
0x7d6ff98, initial_pruning_steps = 0x7d707b0,
exec_pruning_steps = 0x0, execparamids = 0x0}
(gdb) p pd_idx
$3 =
< 2020-08-02 02:04:17.358 SST >LOG: server process (PID 20954) was terminated
by signal 6: Aborted
< 2020-08-02 02:04:17.358 SST >DETAIL: Failed process was running:
INSERT INTO child.cdrs_data_users_per_cell_20200801
(...list of columns elided...)
(
SELECT ..., $3::timestamp, $2,
MODE() WITHIN GROUP (ORDER BY ...) AS ..., STRING_AGG(DISTINCT
..., ',') AS ..., ...
This crashed at 2am, which at first I thought was maybe due to simultaneously
creating today's partition.
Aug 2 02:04:08 telsasoftsky abrt-hook-ccpp: Process 19264 (postgres) of user
26 killed by SIGABRT - dumping core
Aug 2 02:04:17 telsasoftsky abrt-hook-ccpp: Process 20954 (postgres) of user
26 killed by SIGABRT - ignoring
Fix for configure error in 9.5/9.6 on macOS 11.0 Big Sur
Hello,
Under the next version of macOS (11.0 unreleased beta 3), configuring Postgres
9.5 and 9.6 fails with
> checking test program... ok
> checking whether long int is 64 bits... no
> checking whether long long int is 64 bits... no
> configure: error: Cannot find a working 64-bit integer type.
This has been fixed for Postgres 10 and onwards by the following commit. It
would be nice for this to be back-ported for people building 9.5 or 9.6 on
MacOS.
> commit 1c0cf52b39ca3a9a79661129cff918dc000a55eb
> Author: Peter Eisentraut
> Date: Tue Aug 30 12:00:00 2016 -0400
>
> Use return instead of exit() in configure
>
> Using exit() requires stdlib.h, which is not included. Use return
> instead. Also add return type for main().
>
> Reviewed-by: Heikki Linnakangas
> Reviewed-by: Thomas Munro
>
> diff --git a/config/c-compiler.m4 b/config/c-compiler.m4
> index a7f6773ae1..7d901e1f1a 100644
> --- a/config/c-compiler.m4
> +++ b/config/c-compiler.m4
> @@ -71,8 +71,10 @@ int does_int64_work()
> return 0;
>return 1;
> }
> +
> +int
> main() {
> - exit(! does_int64_work());
> + return (! does_int64_work());
> }])],
> [Ac_cachevar=yes],
> [Ac_cachevar=no],
> diff --git a/config/c-library.m4 b/config/c-library.m4
> index d330b0cf95..0a7452c176 100644
> --- a/config/c-library.m4
> +++ b/config/c-library.m4
> @@ -204,8 +204,10 @@ int does_int64_snprintf_work()
> return 0;/* either multiply or snprintf is
> busted */
>return 1;
> }
> +
> +int
> main() {
> - exit(! does_int64_snprintf_work());
> + return (! does_int64_snprintf_work());
> }]])],
> [pgac_cv_snprintf_long_long_int_modifier=$pgac_modifier; break],
> [],
> diff --git a/configure b/configure
> index 55c771a11e..3eb0faf77d 100755
> --- a/configure
> +++ b/configure
> @@ -13594,8 +13594,10 @@ int does_int64_work()
> return 0;
>return 1;
> }
> +
> +int
> main() {
> - exit(! does_int64_work());
> + return (! does_int64_work());
> }
> _ACEOF
> if ac_fn_c_try_run "$LINENO"; then :
> @@ -13676,8 +13678,10 @@ int does_int64_work()
> return 0;
>return 1;
> }
> +
> +int
> main() {
> - exit(! does_int64_work());
> + return (! does_int64_work());
> }
> _ACEOF
> if ac_fn_c_try_run "$LINENO"; then :
> @@ -13770,8 +13774,10 @@ int does_int64_snprintf_work()
> return 0;/* either multiply or snprintf is
> busted */
>return 1;
> }
> +
> +int
> main() {
> - exit(! does_int64_snprintf_work());
> + return (! does_int64_snprintf_work());
> }
> _ACEOF
> if ac_fn_c_try_run "$LINENO"; then :
Kindly,
Thomas Gilligan
thomas.gilli...@icloud.com
Re: Default gucs for EXPLAIN
> On 2 Aug 2020, at 00:12, Justin Pryzby wrote: > On Sun, Aug 02, 2020 at 12:00:56AM +0200, Daniel Gustafsson wrote: >> My reading of this thread and the above that the patch, and CF entry, as it >> stands should be rejected - but that a separate patch for turning BUFFERS on >> by >> default would be highly appreciated. Unless objections I'll go do that in >> the >> CF app for 2020-07. > > Sounds right. Done that way. > I have a patch to enable buffers by default Please attach this thread as well to the CF entry for that patch once registered. cheers ./daniel
Re: Fix for configure error in 9.5/9.6 on macOS 11.0 Big Sur
Thomas Gilligan writes: > Under the next version of macOS (11.0 unreleased beta 3), configuring > Postgres 9.5 and 9.6 fails with >> checking test program... ok >> checking whether long int is 64 bits... no >> checking whether long long int is 64 bits... no >> configure: error: Cannot find a working 64-bit integer type. Hm, could we see the config.log output for this? I'm not 100% convinced that you've diagnosed the problem accurately, because it'd imply that Apple made some fundamentally incompatible changes in libc, which seems like stirring up trouble for nothing. regards, tom lane
Re: OpenSSL randomness seeding
> On 2 Aug 2020, at 09:05, Michael Paquier wrote: > > On Sat, Aug 01, 2020 at 11:48:23PM -0700, Noah Misch wrote: >> On Thu, Jul 30, 2020 at 11:42:16PM +0200, Daniel Gustafsson wrote: >>> Somewhat on topic though, 1.1.1 adds a RAND_priv_bytes function for random >>> numbers that are supposed to be private and extra protected via it's own >>> DRBG. >>> Maybe we should use that for SCRAM salts etc in case we detect 1.1.1? >> >> Maybe. Would you have a separate pg_private_random() function, or just use >> RAND_priv_bytes() for pg_strong_random()? No pg_strong_random() caller is >> clearly disinterested in privacy; gen_random_uuid() may come closest. > > FWIW, I am not sure that we need extra level of complexity when it > comes to random number generation, so having only one API to rule them > all sounds sensible to me, particularly if we know that the API used > has more private protections. I would agree with that, especially since we might not be able to provide an equivalent implementation of a pg_private_random() function in non-OpenSSL builds. Will do a bit more reading and poking and post a patch. cheers ./daniel
Re: MultiXact\SLRU buffers configuration
> On 8 Jul 2020, at 09:03, Andrey M. Borodin wrote: > > > >> 2 июля 2020 г., в 17:02, Daniel Gustafsson написал(а): >> >>> On 15 May 2020, at 02:03, Kyotaro Horiguchi wrote: >> >>> Generally in such cases, condition variables would work. In the >>> attached PoC, the reader side gets no penalty in the "likely" code >>> path. The writer side always calls ConditionVariableBroadcast but the >>> waiter list is empty in almost all cases. But I couldn't cause the >>> situation where the sleep 1000u is reached. >> >> The submitted patch no longer applies, can you please submit an updated >> version? I'm marking the patch Waiting on Author in the meantime. > Thanks, Daniel! PFA V2 This version too has stopped applying according to the CFbot. I've moved it to the next commitfest as we're out of time in this one and it was only pointed out now, but kept it Waiting on Author. cheers ./daniel
Re: doc: vacuum full, fillfactor, and "extra space"
> On 5 Jul 2020, at 13:35, Daniel Gustafsson wrote: > This patch has been Waiting on Author since April, will you have time to > address the questions during this commitfest, or should it be moved to > Returned > with Feedback? This has been closed as Returned with Feedback, please feel free to open a new entry if you return to this work. cheers ./daniel
Re: [Proposal] Add accumulated statistics for wait event
> On 31 Jul 2020, at 07:23, imai.yoshik...@fujitsu.com wrote: > >> This patch fails to apply to HEAD, please submit a rebased version. I've >> marked this as as Waiting on Author. > > Sorry for my absence. Unfortunately I couldn't have time to work on this > patch in this cf. > I believe I will be back in next cf, work on this patch and also review other > patches. No worries, it happens. Since the thread has stalled and there is no updated patch I've marked this entry Returned with Feedback. Please feel free to re-open a new CF entry if you return to this patch. cheers ./daniel
Re: TRUNCATE on foreign tables
> On 2 Jul 2020, at 16:40, Daniel Gustafsson wrote: > >> On 1 Mar 2020, at 03:24, Kohei KaiGai wrote: > >> The attached is revised version. > > This version fails to apply to HEAD due to conflicts in postgres_fdw expected > test output. Can you please submit a rebased version. Marking the entry > Waiting on Author in the meantime. As this has stalled, I've marked this patch Returned with Feedback. Feel free to open a new entry if you return to this patch. cheers ./daniel
Re: Comment simplehash/dynahash trade-offs
On Sun, Aug 2, 2020 at 1:54 PM David Rowley wrote: > On Sun, 2 Aug 2020 at 11:16, David Rowley wrote: > > Maybe it would be better just to get rid of the enum and just #define > > the values. It seems unlikely that we're every going to need many more > > states than what are there already, let along more than, say 127 of > > them. It does look like manifest_file could be shrunk down a bit too > > by making the status field a char. > > This didn't turn out quite as pretty as I had imagined. I needed to > leave the two statuses defined in simplehash.h so that callers could > make use of them. (Result Cache will do this). > > The change here would be callers would need to use SH_STATUS_IN_USE > rather than _SH_IN_USE. > > I'm not really that sold on doing things this way. I really just don't > want to have to make my status field a uint32 in Result Cache per what > the new comment states we must do. If there's a nicer way, then > perhaps that's worth considering. Agreed that the new comment is wrong and should be changed. I think you can probably go further, though, and make it require no storage at all by making it optionally "intrusive", by using a special value in an existing member, and supplying an expression to set and test for that value. For example, maybe some users have a pointer but never want to use NULL, and maybe some users already have a field holding various flags that are one bit wide and can spare a bit.
Re: Comment simplehash/dynahash trade-offs
On Mon, 3 Aug 2020 at 11:10, Thomas Munro wrote: > I think you can probably go further, though, and make it require no > storage at all by making it optionally "intrusive", by using a special > value in an existing member, and supplying an expression to set and > test for that value. For example, maybe some users have a pointer but > never want to use NULL, and maybe some users already have a field > holding various flags that are one bit wide and can spare a bit. I agree that it would be good to allow users of simplehash.h that additional flexibility. It may allow additional memory savings. However, it would mean we'd need to do some additional work when we create and grow the hash table to ensure that we mark new buckets as empty. For now, we get that for free with the zeroing of the newly allocated memory, but we couldn't rely on that if we allowed users to define their own macro. It looks like none of the current callers could gain from this 1. TupleHashEntryData does not have any reusable fields. The status should fit in the padding on a 64-bit machine anyway. 2. PagetableEntry already has a status that fits into the padding. 3. manifest_file could have its status moved to the end of the struct and made into a char and the struct would be the same size as if the field did not exist. So, with the current users, we'd stand to lose more than we'd gain from doing it that way. David
Re: Comment simplehash/dynahash trade-offs
On Mon, 3 Aug 2020 at 11:36, David Rowley wrote: > So, with the current users, we'd stand to lose more than we'd gain > from doing it that way. FWIW, I'd be ok with just: - * The element type is required to contain a "uint32 status" member. + * The element type is required to contain an integer-based "status" member + * which can store the range of values defined in the SH_STATUS enum. David
Re: LDAP check flapping on crake due to race
On Mon, Aug 3, 2020 at 4:09 AM Tom Lane wrote:
> I'm inclined to suggest that the LDAP test's test_access could use
> an empty stdin and pass "-c 'SELECT 1'" as a command line option
> instead. (Maybe that's exactly what you meant, but I'm not sure.)
Good idea. Here's a patch like that.
> I've not been able to duplicate this locally, so I have no idea if
> that'd really fix it.
Me neither -- I guess someone who enjoys perl could hack IPC::Run to
take a short nap at the right moment.
From 0d01b65cef920c953551ce1c49d071fdf29f Mon Sep 17 00:00:00 2001
From: Thomas Munro
Date: Mon, 3 Aug 2020 11:34:40 +1200
Subject: [PATCH] Fix rare failure in LDAP tests.
Instead of writing a query to psql's stdin, use -c. This avoids a
failure where psql exits before we write, seen a few times on the build
farm. Thanks to Noah Misch and Tom Lane for suggesting this fix.
Back-patch to 11, where the LDAP tests arrived.
Discussion: https://postgr.es/m/CA%2BhUKGLFmW%2BHQYPeKiwSp5sdFFHtFViCpw4Mh6yAgEx74r5-Cw%40mail.gmail.com
---
src/test/ldap/t/001_auth.pl | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/test/ldap/t/001_auth.pl b/src/test/ldap/t/001_auth.pl
index f8941144f5..3bc7672451 100644
--- a/src/test/ldap/t/001_auth.pl
+++ b/src/test/ldap/t/001_auth.pl
@@ -165,7 +165,8 @@ sub test_access
my ($node, $role, $expected_res, $test_name) = @_;
my $res =
- $node->psql('postgres', 'SELECT 1', extra_params => [ '-U', $role ]);
+ $node->psql('postgres', undef,
+ extra_params => [ '-U', $role, '-c', 'SELECT 1' ]);
is($res, $expected_res, $test_name);
return;
}
--
2.20.1
dblnk_is_busy returns 1 for dead connecitons
Hackers, I have a situation that I am observing where dblink_is_busy returns 1 even though the connection is long gone. tcp keepalives are on and the connection has been dead for several hours. Looking at the call for dblink_is_busy, I see that it is a thin wrapper to PQusBusy(). If I attempt to call dblink_get_result(), the result comes back with an error mesage, 'invalid socket'. This however is not helpful since there is no way to probe for dead connections in dblink that appears to be 100% reliable. My workaround that I had been relying on was to call dblink_get_notify twice, which for some weird reason forced the connection error to the surface. However for whatever reason, that is not working here. In cases the connection was cancelled via dblink_cancel_query(), so in some scenarios connections cancelled that way seem to become 'stuck'. Any thoughts on this? merlin
Re: Comment simplehash/dynahash trade-offs
On Mon, Aug 3, 2020 at 11:42 AM David Rowley wrote: > On Mon, 3 Aug 2020 at 11:36, David Rowley wrote: > > So, with the current users, we'd stand to lose more than we'd gain > > from doing it that way. > > FWIW, I'd be ok with just: > > - * The element type is required to contain a "uint32 status" member. > + * The element type is required to contain an integer-based > "status" member > + * which can store the range of values defined in the SH_STATUS enum. Thanks for the correction. Pushed.
Re: LDAP check flapping on crake due to race
On Mon, Aug 03, 2020 at 12:12:57PM +1200, Thomas Munro wrote: > On Mon, Aug 3, 2020 at 4:09 AM Tom Lane wrote: > > I'm inclined to suggest that the LDAP test's test_access could use > > an empty stdin and pass "-c 'SELECT 1'" as a command line option > > instead. (Maybe that's exactly what you meant, but I'm not sure.) > > Good idea. Here's a patch like that. While I had meant a different approach, this is superior. > > I've not been able to duplicate this locally, so I have no idea if > > that'd really fix it. > > Me neither -- I guess someone who enjoys perl could hack IPC::Run to > take a short nap at the right moment. Not essential to reproduce first, I think.
Re: LDAP check flapping on crake due to race
On Mon, Aug 3, 2020 at 12:29 PM Noah Misch wrote: > On Mon, Aug 03, 2020 at 12:12:57PM +1200, Thomas Munro wrote: > > On Mon, Aug 3, 2020 at 4:09 AM Tom Lane wrote: > > > I'm inclined to suggest that the LDAP test's test_access could use > > > an empty stdin and pass "-c 'SELECT 1'" as a command line option > > > instead. (Maybe that's exactly what you meant, but I'm not sure.) > > > > Good idea. Here's a patch like that. > > While I had meant a different approach, this is superior. Thanks. Pushed.
Re: psql - improve test coverage from 41% to 88%
On Sun, Aug 02, 2020 at 11:10:23AM -0400, Andrew Dunstan wrote: > +1. Also note that the Windows animals don't and probably will never > support Expect, since Windows doesn't have PTYs. Expect.pm is in fact a > pure perl module that sits on top of IO::Pty, which in turn sits on top > of IO::Tty. So if you have those Expect.pm probably isn't a huge > stretch. But let's not add a dependency if we can avoid it. And if we do > add one it will need to be a soft one like the case you mentioned. Even with that, do we really care about some code coverage specific to Windows for tab-complete.c? Also, how complicated does the proposed patch become if we remove the dependency to Expect.pm and just rely on IO::Pty? -- Michael signature.asc Description: PGP signature
Re: display offset along with block number in vacuum errors
On Sun, 2 Aug 2020 at 13:24, Justin Pryzby wrote: > > On Sun, Aug 02, 2020 at 01:02:42PM +0900, Masahiko Sawada wrote: > > Thank you for updating the patch! > > > > Here are my comments on v3 patch: > > > > @@ -2024,6 +2033,11 @@ lazy_check_needs_freeze(Buffer buf, bool *hastup) > > if (PageIsNew(page) || PageIsEmpty(page)) > > return false; > > > > + /* Update error traceback information */ > > + update_vacuum_error_info(vacrelstats, &saved_err_info, > > + VACUUM_ERRCB_PHASE_SCAN_HEAP, vacrelstats->blkno, > > + InvalidOffsetNumber); > > + > > maxoff = PageGetMaxOffsetNumber(page); > > for (offnum = FirstOffsetNumber; > > offnum <= maxoff; > > > > You update the error callback phase to VACUUM_ERRCB_PHASE_SCAN_HEAP > > but I think we're already in that phase. I'm okay with explicitly > > setting it but on the other hand, we don't set the phase in > > heap_page_is_all_visible(). Is there any reason for that? > > That part was my suggestion, so I can answer that. I added > update_vacuum_error_info() to lazy_check_needs_freeze() to allow it to later > call restore_vacuum_error_info(). > > > Also, since we don't reset vacrelstats->offnum at the end of > > heap_page_is_all_visible(), if an error occurs by the end of > > lazy_vacuum_page(), the caller of heap_page_is_all_visible(), we > > report the error context with the last offset number in the page, > > making the users confused. > > So it looks like heap_page_is_all_visible() should also call the update and > restore functions. > > Do you agree with my suggestion that the VACUUM phase should never try to > report an offset ? Yeah, given the current heap vacuum implementation, I agree that setting the offset number during VACUUM_HEAP phase doesn't help anything. But setting the offset number during checking tuples' visibility in heap_page_is_all_visible() might be useful, although it might be unlikely to find a problem in heap_page_is_all_visible() as the tuple visibility checking is already done in lazy_scan_heap(). I wonder if we can set SCAN_HEAP phase and update the offset number in heap_page_is_all_visible(). > How do you think we can phrase the message to avoid confusion due to 0-based > block number and 1-based offset ? I think that since the user who uses this errcontext information is likely to know more or less the internal of PostgreSQL I think 0-based block number and 1-based offset will not be a problem. However, I expected these are documented but couldn't find. If not yet, I think it’s a good time to document that. Regards, -- Masahiko Sawadahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: dblnk_is_busy returns 1 for dead connecitons
On Sun, Aug 2, 2020 at 7:18 PM Merlin Moncure wrote: > > Hackers, > > I have a situation that I am observing where dblink_is_busy returns 1 > even though the connection is long gone. tcp keepalives are on and > the connection has been dead for several hours. Looking at the call > for dblink_is_busy, I see that it is a thin wrapper to PQusBusy(). > If I attempt to call dblink_get_result(), the result comes back with > an error mesage, 'invalid socket'. This however is not helpful since > there is no way to probe for dead connections in dblink that appears > to be 100% reliable. My workaround that I had been relying on was to > call dblink_get_notify twice, which for some weird reason forced the > connection error to the surface. However for whatever reason, that is > not working here. > > In cases the connection was cancelled via dblink_cancel_query(), so in > some scenarios connections cancelled that way seem to become 'stuck'. > Any thoughts on this? Correction, keepalives are probably not on, because dblink does not have an option to set them. Also, it looks like dblink_is_busy() calls pqConsumeInput without checking the error code. Is that safe? merlin
Re: new heapcheck contrib module
> On Jul 30, 2020, at 10:59 AM, Robert Haas wrote: > > + curchunk = DatumGetInt32(fastgetattr(toasttup, 2, > + ctx->toast_rel->rd_att, &isnull)); > > Should we be worrying about the possibility of fastgetattr crapping > out if the TOAST tuple is corrupted? I think we should, but I'm not sure we should be worrying about it at this location. If the toast index is corrupt, systable_getnext_ordered could trip over the index corruption in the process of retrieving the toast tuple, so checking the toast tuple only helps if the toast index does not cause a crash first. I think the toast index should be checked before this point, ala verify_nbtree, so that we don't need to worry about that here. It might also make more sense to verify the toast table ala verify_heapam prior to here, so we don't have to worry about that here either. But that raises questions about whose responsibility this all is. If verify_heapam checks the toast table and toast index before the main table, that takes care of it, but makes a mess of the idea of verify_heapam taking a start and end block, since verifying the toast index is an all or nothing proposition, not something to be done in incremental pieces. If we leave verify_heapam as it is, then it is up to the caller to check the toast before the main relation, which is more flexible, but is more complicated and requires the user to remember to do it. We could split the difference by having verify_heapam do nothing about toast, leaving it up to the caller, but make pg_amcheck handle it by default, making it easier for users to not think about the issue. Users who want to do incremental checking could still keep track of the chunks that have already been checked, not just for the main relation, but for the toast relation, too, and give start and end block arguments to verify_heapam for the toast table check and then again for the main table check. That doesn't fix the question of incrementally checking the index, though. Looking at it a slightly different way, I think what is being checked at the point in the code you mention is the logical structure of the toasted value related to the current main table tuple, not the lower level tuple structure of the toast table. We already have a function for checking a heap, namely verify_heapam, and we (or the caller, really) should be using that. The clean way to do things is verify_heapam(toast_rel) verify_btreeam(toast_idx) verify_heapam(main_rel) and then depending on how fast and loose you want to be, you can use the start and end block arguments, which are inherently a bit half-baked, given the lack of any way to be sure you check precisely the right range of blocks, and also you can be fast and loose about skipping the index check or not, as you see fit. Thoughts? — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Document "59.2. Built-in Operator Classes" have a clerical error?
hi all: In Document "Table 59-1. Built-in GiST Operator Classes": "range_opsany range type&& &> &< >> << <@ -|- = @> @>"， exist double "@>"， Should be "<@ @>" ?
Re: Document "59.2. Built-in Operator Classes" have a clerical error?
On Sun, Aug 2, 2020 at 8:17 PM osdba wrote: > hi all: > > In Document "Table 59-1. Built-in GiST Operator Classes": > > "range_ops any range type && &> &< >> << <@ -|- = @> @>"， exist double " > @>"， > > Should be "<@ @>" ? > > It helps to reference the current version of the page (or provide a url link) as that section seems to have migrated to Chapter 64 - though it is unchanged even on the main development branch. The table itself is extremely difficult to read: it would be more easily readable if the font was monospaced, but its not. I'm reasonably confident that the equal sign is part of the second-to-last operator while the lone @> is the final operator. Mostly I say this because GiST doesn't do straight equality so a lone equal operator isn't valid. David J.
Re: new heapcheck contrib module
On Mon, Jul 27, 2020 at 10:02 AM Mark Dilger wrote: > I'm indifferent about that change. Done for v13. Moving on with verification of the same index in the event of B-Tree index corruption is a categorical mistake. verify_nbtree.c was simply not designed to work that way. You were determined to avoid allowing any behavior that can result in a backend crash in the event of corruption, but this design will defeat various measures I took to avoid crashing with corrupt data (e.g. in commit a9ce839a313). What's the point in not just giving up on the index (though not necessarily the table or other indexes) at the first sign of trouble, anyway? It makes sense for the heap structure, but not for indexes. -- Peter Geoghegan
Re: new heapcheck contrib module
On Thu, Jul 30, 2020 at 10:59 AM Robert Haas wrote: > I don't really like the name, either. I get that it's probably > inspired by Perl, but I think it should be given a less-clever name > like report_corruption() or something. +1 -- confess() is an awful name for this. -- Peter Geoghegan
Re: expose parallel leader in CSV and log_line_prefix
On Fri, Jul 31, 2020 at 10:31:13PM -0500, Justin Pryzby wrote: > Also, I was reminded by Tom's c410af098 about this comment: > > * Further note: At least on some platforms, passing %*s > rather than > * %s to appendStringInfo() is substantially slower, so many > of the > * cases below avoid doing that unless non-zero padding is in > fact > * specified. > > It seems we can remove that hack and avoid its spiriling conditionals. > It's cleaner to make that 0001. Not sure what 0001 is doing on this thread, so I would suggest to create a new thread for that to attract the correct audience. It is true that we should not need that anymore as we use our own implementation of sprintf now. For now, I have taken 0002 as a base, fixed a couple of things (doc tweaks, removed unnecessary header inclusion, etc.), and committed it, meaning that we are done here. -- Michael signature.asc Description: PGP signature
[PATCH v1] elog.c: Remove special case which avoided %*s format strings..
..which should no longer be needed since it was a performance hack for specific
platform snprintf, which are no longer used.
>From dacde652145e65e8de0a0cb7349c9f5f10314243 Mon Sep 17 00:00:00 2001
From: Justin Pryzby
Date: Fri, 31 Jul 2020 21:53:01 -0500
Subject: [PATCH v1] elog.c: Remove special case which avoided %*s format
strings..
..which should no longer be needed since it was a performance hack for specific
platform snprintf, which are no longer used.
See also:
4334639f4 Allow printf-style padding specifications in log_line_prefix.
96bf88d52 Always use our own versions of *printf().
abd9ca377 Make assorted performance improvements in snprintf.c.
---
src/backend/utils/error/elog.c | 134 -
1 file changed, 32 insertions(+), 102 deletions(-)
diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index d0b368530e..6b6749965f 100644
--- a/src/backend/utils/error/elog.c
+++ b/src/backend/utils/error/elog.c
@@ -2350,11 +2350,6 @@ log_line_prefix(StringInfo buf, ErrorData *edata)
* Note: Since only '-', '0' to '9' are valid formatting characters we
* can do a quick check here to pre-check for formatting. If the char
* is not formatting then we can skip a useless function call.
- *
- * Further note: At least on some platforms, passing %*s rather than
- * %s to appendStringInfo() is substantially slower, so many of the
- * cases below avoid doing that unless non-zero padding is in fact
- * specified.
*/
if (*p > '9')
padding = 0;
@@ -2371,10 +2366,7 @@ log_line_prefix(StringInfo buf, ErrorData *edata)
if (appname == NULL || *appname == '\0')
appname = _("[unknown]");
- if (padding != 0)
- appendStringInfo(buf, "%*s", padding, appname);
- else
- appendStringInfoString(buf, appname);
+ appendStringInfo(buf, "%*s", padding, appname);
}
else if (padding != 0)
appendStringInfoSpaces(buf,
@@ -2392,10 +2384,7 @@ log_line_prefix(StringInfo buf, ErrorData *edata)
else
backend_type_str = GetBackendTypeDesc(MyBackendType);
- if (padding != 0)
- appendStringInfo(buf, "%*s", padding, backend_type_str);
- else
- appendStringInfoString(buf, backend_type_str);
+ appendStringInfo(buf, "%*s", padding, backend_type_str);
break;
}
case 'u':
@@ -2405,10 +2394,7 @@ log_line_prefix(StringInfo buf, ErrorData *edata)
if (username == NULL || *username == '\0')
username = _("[unknown]");
- if (padding != 0)
- appendStringInfo(buf, "%*s", padding, username);
- else
- appendStringInfoString(buf, username);
+ appendStringInfo(buf, "%*s", padding, username);
}
else if (padding != 0)
appendStringInfoSpaces(buf,
@@ -2421,17 +2407,13 @@ log_line_prefix(StringInfo buf, ErrorData *edata)
if (dbname == NULL || *dbname == '\0')
dbname = _("[unknown]");
- if (padding != 0)
- appendStringInfo(buf, "%*s", padding, dbname);
- else
- appendStringInfoString(buf, dbname);
+ appendStringInfo(buf, "%*s", padding, dbname);
}
else if (padding != 0)
appendStringInfoSpaces(buf,
padding > 0 ? padding : -padding);
break;
case 'c':
-if (padding != 0)
{
char strfbuf[128];
@@ -2439,14 +2421,9 @@ log_line_prefix(StringInfo buf, ErrorData *edata)
(long) (MyStartTime), MyProcPid);
appendStringInfo(buf, "%*s", padding, strfbuf);
}
-else
- appendStringInfo(buf, "%lx.%x", (long) (MyStartTime), MyProcPid);
break;
case 'p':
-if (padding != 0)
- appendStringInfo(buf, "%*d", padding, MyProcPid);
-else
- appendStringInfo(buf, "%d", MyProcPid);
+appendStringInfo(buf, "%*d", padding, MyProcPid);
break;
case 'P':
@@ -2472,17 +2449,11 @@ log_line_prefix(StringInfo buf, ErrorData *edata)
break;
case 'l':
-if (padding != 0)
- appendStringInfo(buf, "%*ld", padding, log_line_number);
-else
- appendStringInfo(buf, "%ld", log_line_number);
+appendStringInfo(buf, "%*ld", padding, log_line_number);
break;
case 'm':
setup_formatted_log_time();
-if (padding != 0)
- appendStringInfo(buf, "%*s", padding, formatted_log_time);
-else
- appendStringInfoString(buf, formatted_log_time);
+appendStringInfo(buf, "%*s", padding, formatted_log_time);
break;
case 't':
{
@@ -2492,10 +2463,7 @@ log_line_prefix(StringInfo buf, ErrorData *edata)
pg_strftime(strfbuf, sizeof(strfbuf),
"%Y-%m-%d %H:%M:%S %Z",
pg_localtime(&stamp_time, log_timezone));
- if (padding != 0)
- appendStringInfo(buf, "%*s", padding, strfbuf);
- else
- appendStringInfoString(buf, strfbuf);
+ appendStringInfo(buf, "%*s", padding, strfbuf);
}
break;
case 'n':
@@ -2512,19 +2480,13 @@ log_line_prefix(StringInfo buf, ErrorData *edata)
(long) saved_timeval.tv_sec,
(int) (saved_tim
Re: problem with RETURNING and update row movement
Fujita-san,
Thanks for your time on this.
On Sun, Aug 2, 2020 at 5:57 PM Etsuro Fujita wrote:
> Yet another thing I noticed is that the patch incorrectly produces
> values for the tableoid columns specified in the RETURNING list, like
> this:
Yeah, I noticed that too.
> +UPDATE range_parted r set a = 'c' FROM (VALUES ('a', 1), ('a', 10),
> ('b', 12)) s(x, y) WHERE s.x = r.a AND s.y = r.b RETURNING
> tableoid::regclass, *;
> +tableoid| a | b | c | d | e | x | y
> ++---++-+---+---+---+
> + part_a_1_a_10 | c | 1 | 1 | 1 | in trigfunc() | a | 1
> + part_a_10_a_20 | c | 10 | 200 | 1 | in trigfunc() | a | 10
> + part_c_1_100 | c | 12 | 96 | 1 | in trigfunc() | b | 12
> +(3 rows)
>
> The source partitions are shown as tableoid, but the destination
> partition (ie, part_c_1_c_20) should be shown. To fix this, I
> modified the patch further so that 1) we override tts_tableOid of the
> original slot with the OID of the destination partition before calling
> ExecProcessReturning() if needed, and 2) in ExecProcessReturning(), we
> only initialize ecxt_scantuple's tts_tableOid when needed, which would
> save cycles a bit for non-foreign-table-direct-modification cases.
>
> Attached is a new version of the patch.
Thanks for the updated patch. I reviewed your changes in v3 too and
they looked fine to me.
However, I noticed that having system columns like ctid, xmin, etc. in
the RETURNING list is now broken and maybe irrepairably due to the
approach we are taking in the patch. Let me show an example:
drop table foo;
create table foo (a int, b int) partition by list (a);
create table foo1 (c int, b int, a int);
alter table foo1 drop c;
alter table foo attach partition foo1 for values in (1);
create table foo2 partition of foo for values in (2);
create table foo3 partition of foo for values in (3);
create or replace function trigfunc () returns trigger language
plpgsql as $$ begin new.b := 2; return new; end; $$;
create trigger trig before insert on foo2 for each row execute
function trigfunc();
insert into foo values (1, 1), (2, 2), (3, 3);
update foo set a = 2 from (values (1), (2), (3)) s(x) where a = s.x
returning tableoid::regclass, ctid, xmin, xmax, *;
tableoid | ctid | xmin |xmax| a | b | x
--++--++---+---+---
foo2 | (4294967295,0) | 128 | 4294967295 | 2 | 2 | 1
foo2 | (0,3) | 782 | 0 | 2 | 2 | 2
foo2 | (0,4) | 782 | 0 | 2 | 2 | 3
(3 rows)
During foo1's update, it appears that we are losing the system
information in the physical tuple initialized during ExecInsert on
foo2 during its conversion back to foo1's reltype using the new code.
I haven't been able to figure out how to preserve the system
information in HeapTuple contained in the destination slot across the
conversion. Note we want to use the latter to project the RETURNING
list.
By the way, you'll need two adjustments to even get this example
working, one of which is a reported problem that system columns in
RETURNING list during an operation on partitioned table stopped
working in PG 12 [1] for which I've proposed a workaround (attached).
Another is that we forgot in our patch to "materialize" the virtual
tuple after conversion, which means slot_getsysattr() can't find it to
access system columns like xmin, etc.
--
Amit Langote
EnterpriseDB: http://www.enterprisedb.com
[1] https://www.postgresql.org/message-id/141051591267657%40mail.yandex.ru
use-heap-slot-for-partitioned-table.patch
Description: Binary data
Re: public schema default ACL
On Fri, Mar 23, 2018 at 07:47:39PM -0700, Noah Misch wrote: > In light of the mixed reception, I am withdrawing this proposal. I'd like to reopen this. Reception was mixed, but more in favor than against. Also, variations on the idea trade some problems for others and may be more attractive. The taxonomy of variations has three important dimensions: Interaction with dump/restore (including pg_upgrade) options: a. If the schema has a non-default ACL, dump/restore reproduces it. Otherwise, the new default prevails. b. Dump/restore always reproduces the schema ACL. Initial ownership of schema "public" options: 1. Bootstrap superuser owns it. (Without superuser cooperation, database owners can't drop it or create objects in it.) 2. Don't create the schema during initdb. Database owners can create it or any other schema. (A superuser could create it in template1, which converts an installation to option (1).) 3. Database owner owns it. (One might implement this by offering ALTER SCHEMA x OWNER TO DATABASE_OWNER, which sets nspowner to a system OID meaning "refer to pg_database.datdba". A superuser could issue DDL to convert to option (1) or (2).) Automatic creation of $user schemas options: X. Automatic schema creation doesn't exist. Y. Create $user schemas on-demand (at login time or CREATE TABLE/CREATE FUNCTION/etc. time) if the DBA specified a "SCHEMA_CREATE" option in the CREATE ROLE statement. Z. Like (Y), but SCHEMA_CREATE is the default. I started the thread by proposing (a)(1)(X) and mentioning (b)(1)(X) as an alternative. Given the compatibility concerns, I now propose ruling out (a) in favor of (b). http://postgr.es/m/0e61bd66-07a2-255b-2b0f-7a8488ea1...@2ndquadrant.com identified (b)(2)(X) and identified the problem with (1). I dislike (Z), because it requires updating security guidelines to specify NOSCHEMA_CREATE; I think it would be better to leave $SUBJECT unchanged than to adopt (Z). I like (Y) from an SQL standard perspective, but I don't think it resolves the ease-of-first-use objections raised against (a)(1)(X). (If changing the public schema ACL is too much of an obstacle for a DBA, adopting SCHEMA_CREATE is no easier.) Hence, I propose ruling out (Y) and (Z). That leaves the choice between (2) and (3). Under (b)(2)(X), first-use guides would need to add some CREATE SCHEMA. While (3) avoids that, some users may find themselves setting ownership back to the bootstrap superuser. (3) also makes the system more complex overall. Between (b)(2)(X) and (b)(3)(X), what are folks' preferences? Does anyone strongly favor some other option (including the option of changing nothing) over both of those two? Thanks, nm
Re: Add MAIN_RELATION_CLEANUP and SECONDARY_RELATION_CLEANUP options to VACUUM
On Tue, Jul 14, 2020 at 05:34:01AM +, k.jami...@fujitsu.com wrote: > I've also confirmed those through regression + tap test in my own env > and they've passed. I'll look into deeply again if I find problems. + VACOPT_TOAST_CLEANUP = 1 << 6, /* process TOAST table, if any */ + VACOPT_DISABLE_PAGE_SKIPPING = 1 << 7, /* don't skip any pages */ + VACOPT_MAIN_REL_CLEANUP = 1 << 8/* process main relation */ } VacuumOption; Do we actually need this much complication in the option set? It is possible to vacuum directly a toast table by passing directly its relation name, with pg_toast as schema, so you can already vacuum a toast relation without the main part. And I would guess that users caring about the toast table specifically would know already how to do that, even if it requires a simple script and a query on pg_class. Now there is a second part, where we'd like to vacuum the main relation but not its toast table. My feeling by looking at this patch today is that we could just make VACOPT_SKIPTOAST an option available at user-level, and support all the cases discussed on this thread. And we have already all the code in place to support that in the backend for autovacuum as relations are processed individually, without their toast tables if they have one. > I think this follows the similar course of previously added VACUUM and > vacuummdb options (for using and skipping truncate, index cleanup, etc.), > so the patch seems almost plausible enough for me. -static bool vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params); +static bool vacuum_rel(Oid relid, + RangeVar *relation, + VacuumParams *params, + bool processing_toast_table); Not much a fan of the addition of this parameter on this routine to track down if the call should process a toast relation or not. Couldn't you just prevent the call to vacuum_rel() to happen at all? -- Michael signature.asc Description: PGP signature
Re: Make autovacuum sort tables in descending order of xid_age
On Mon, Mar 30, 2020 at 09:20:15AM -0700, Mark Dilger wrote: > I have not been working on this issue lately, but as I recall, my > concern was that changing the behavior of autovacuum could introduce > regressions for some users, so we should be careful to get it right > before we rush to release anything. It didn't seem like the > proposed changes took enough into account. But that's clearly a > judgement call, having to do with how cautious any particular person > thinks we should be. I don't feel strongly enough to stand in the > way if the general concensus is that this is a good enough > implementation. Echoing with what has been already mentioned on this thread, I think that autovacuum scheduling is a hard problem, and I would be rather scared to change by default a behavior that has proved to work in some cases, but could potentially doom others. I have an idea though: we could make the scheduling behavior of autovacuum optional. Anyway, the thread has stalled for a couple of months now, and we don't have a clear consensus about this approach, so I am marking this thread as returned with feedback. -- Michael signature.asc Description: PGP signature
