[HACKERS] Fix comment in ATExecValidateConstraint
The comment seems to have been copied from ATExecAddColumn, which says: /* * If we are told not to recurse, there had better not be any - * child tables; else the addition would put them out of step. For ATExecValidateConstraint, it should say something like: + * child tables; else validating the constraint would put them + * out of step. Attached patch fixes it. Thanks, Amit diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 86e9814..ddb2f2a 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -6908,7 +6908,8 @@ ATExecValidateConstraint(Relation rel, char *constrName, bool recurse, /* * If we are told not to recurse, there had better not be any - * child tables; else the addition would put them out of step. + * child tables; else validating the constraint would put them + * out of step. */ if (!recurse) ereport(ERROR, -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Constraint merge and not valid status
Hello, At Mon, 25 Jul 2016 15:57:00 +0900, Amit Langote wrote in > On 2016/07/25 12:44, Kyotaro HORIGUCHI wrote: > > At Fri, 22 Jul 2016 17:35:48 +0900, Amit Langote wrote: > >> On 2016/07/22 17:06, Kyotaro HORIGUCHI wrote: > >> > >>> By the way I have one question. > >>> > >>> Is it an expected configuration where tables in an inheritance > >>> tree has different valid state on the same (check) constraint? > >> > >> I would think not. > > > > I understand that the first problem is that the problematic > > state inhibits later VALIDATE CONSTRAINT on parent from working > > as expected. This patch inhibits the state where a parent is > > valid and any of its children is not-valid, but allows the > > opposite and it is enough to fix the problem. > > > > I thought the opposite state is ok generally but not with full > > confidence. > > I obviously missed the opposite case. However, it's OK for a child's > constraint to be valid while the parent's is not. There seems to be in > place only the one-way rule which is that we don't mark parent's > constraint valid until and unless it is marked valid on all the child > tables. From the following comment in ATExecValidateConstraint(): > > /* > * For CHECK constraints, we must ensure that we only mark the > * constraint as validated on the parent if it's already validated > * on the children. > * > > And it seems to be in place only for VALIDATE CONSTRAINT's purpose. Agreed. It guarantees nothing outside the function but it shows that at least one side of mixed validity status is/should be considered. > > After some reading the code, it seems to affect only on some > > cache invalidation logics and constraint exclusion logic to > > ignore the check constraint per component table, and > > acquire_inherited_sample_rows. > > > > The first and second wouldn't harm. The third causes needless > > tuple conversion. If this is a problem, the validity state of all > > relations in an inheritance tree should be exactly the same, > > ether valid or not-valid. Or should make the function to ignore > > the difference of validity state. > > Hmm, perhaps check constraint valid status affecting whether > child-to-parent-rowtype conversion should occur is unnecessary. Maybe a > subset of those checks for purposes of acquire_inherited_sample_rows would > suffice. Or simply make equalTupleDescs accept a boolean parameter that > indicates whether or not to check TupleConstr equality. equalTupleDescs are used in few places. The equalTupleDescs's "Logical" equality seems to stem from the compatibility while given to heap_form/modify_tuple. I don't think the validity status has something to do with the result of such functions. > > If the problem is only VALIDATE CONSTRAINT on the parent and > > mixted validity states within an inheritance tree is not, making > > it process whole the inheritance tree regardsless of the validity > > state of the parent would also fix the problem. > > > > After all, my concerns are the following. > > > > - Is the mixed validity states (in any form) in an inheritnce > >tree should be valid? If so, VALIDATE CONSTRAINT should be > >fixed, not MergeWithExistingConstraint. If not, the opposite > >state also should be inhibited. > > > > - Is it valid to represent all descendants' validity states by > >the parent's state? (Currently only VALIDATE CONSTRAINT does) > >If not, VALIDATE CONSTRAINT should be fixed. > > > > Any thoughts? > > Wouldn't it be better to leave VALIDATE CONSTRAINT alone, because the way > it works now it short-circuits some extra processing if the parent's > constraint is marked valid? All we need to do is to prevent the rule from > being broken by fixing the current code like the patch proposes. If we > try to fix the VALIDATE CONSTRAINT instead, we'll always have to pay the > cost of find_all_inheritors(). Thoughts? VALIDATE CONSTRAINT is expected to take a long time like analyze and used for very limited cases so the cost of find_all_inheritors() don't seem to be siginificant. However, such modification would give a pain more than required. As the result, I agree with you. > As for the other cases (determining whether we can live with the state > where a child's constraint is valid whereas the same on the parent and > siblings is not): > > 1. Both cache invalidation and acquire_inherited_sample_rows depend on > equalTupleDescs, where the former is unrelated to inheritance behavior as > such (and hence unaffected by current discussion); for the latter, we > might want to simply ignore comparing the check constraint valid status as > mentioned above > > 2. Constraint exclusion, where it seems OK for a child's check constraint > to cause it to be excluded while the same check constraint of its parent > and siblings is ignored causing them to be needlessly scanned. Agreed to the both. So the conclusion here is, - Inhibit only the case where parent is to be validated while child is
Re: [HACKERS] Fix typo in postgres_fdw/deparse.c
On Thu, Jul 14, 2016 at 2:22 PM, Masahiko Sawada wrote: > Hi all, > > Attached patch fixes small typo in contrib/postgres_fdw/deparse.c > > s/whenver/whenever/g Applied. Thanks! Regards, -- Fujii Masao -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Constraint merge and not valid status
Hello,
On 2016/07/25 17:18, Kyotaro HORIGUCHI wrote:
>
> - Remove ccvalid condition from equalTupleDescs() to reduce
>unnecessary cache invalidation or tuple rebuilding.
The following commit introduced the ccvalid check:
"""
commit c31305de5f5a4880b0ba2f5983025ef0210a3b2a
Author: Noah Misch
Date: Sun Mar 23 02:13:43 2014 -0400
Address ccvalid/ccnoinherit in TupleDesc support functions.
equalTupleDescs() neglected both of these ConstrCheck fields, and
CreateTupleDescCopyConstr() neglected ccnoinherit. At this time, the
only known behavior defect resulting from these omissions is constraint
exclusion disregarding a CHECK constraint validated by an ALTER TABLE
VALIDATE CONSTRAINT statement issued earlier in the same transaction.
Back-patch to 9.2, where these fields were introduced.
"""
So, apparently RelationClearRelation() does intend to discard a cached
TupleDesc if ccvalid changed in a transaction. Whereas,
acquire_inherited_sample_rows() does not seem to depend on ccvalid being
equal or not (or any member of TupleConstr for that matter). So maybe,
instead of simply discarding the check (which does serve a purpose), we
could make equalTupleDescs() parameterized on whether we want TupleConstr
equality check to be performed or not. RelationClearRelation() can ask
for the check to be performed by passing true for the parameter whereas
acquire_inherited_sample_rows() and other callers can pass false. Perhaps
something like the attached.
Thanks,
Amit
diff --git a/src/backend/access/common/tupdesc.c b/src/backend/access/common/tupdesc.c
index b56d0e3..3dcb656 100644
--- a/src/backend/access/common/tupdesc.c
+++ b/src/backend/access/common/tupdesc.c
@@ -348,7 +348,7 @@ DecrTupleDescRefCount(TupleDesc tupdesc)
* We don't compare tdrefcount, either.
*/
bool
-equalTupleDescs(TupleDesc tupdesc1, TupleDesc tupdesc2)
+equalTupleDescs(TupleDesc tupdesc1, TupleDesc tupdesc2, bool compare_constr)
{
int i,
j,
@@ -411,6 +411,9 @@ equalTupleDescs(TupleDesc tupdesc1, TupleDesc tupdesc2)
/* attacl, attoptions and attfdwoptions are not even present... */
}
+ if (!compare_constr)
+ return true;
+
if (tupdesc1->constr != NULL)
{
TupleConstr *constr1 = tupdesc1->constr;
diff --git a/src/backend/catalog/pg_proc.c b/src/backend/catalog/pg_proc.c
index c1d1505..7ce2da5 100644
--- a/src/backend/catalog/pg_proc.c
+++ b/src/backend/catalog/pg_proc.c
@@ -434,7 +434,7 @@ ProcedureCreate(const char *procedureName,
if (olddesc == NULL && newdesc == NULL)
/* ok, both are runtime-defined RECORDs */ ;
else if (olddesc == NULL || newdesc == NULL ||
- !equalTupleDescs(olddesc, newdesc))
+ !equalTupleDescs(olddesc, newdesc, false))
ereport(ERROR,
(errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
errmsg("cannot change return type of existing function"),
diff --git a/src/backend/commands/analyze.c b/src/backend/commands/analyze.c
index 5fcedd7..f96333d 100644
--- a/src/backend/commands/analyze.c
+++ b/src/backend/commands/analyze.c
@@ -1417,7 +1417,8 @@ acquire_inherited_sample_rows(Relation onerel, int elevel,
/* We may need to convert from child's rowtype to parent's */
if (childrows > 0 &&
!equalTupleDescs(RelationGetDescr(childrel),
- RelationGetDescr(onerel)))
+ RelationGetDescr(onerel),
+ false))
{
TupleConversionMap *map;
diff --git a/src/backend/utils/cache/plancache.c b/src/backend/utils/cache/plancache.c
index f42a62d..d3b0d6c 100644
--- a/src/backend/utils/cache/plancache.c
+++ b/src/backend/utils/cache/plancache.c
@@ -711,7 +711,7 @@ RevalidateCachedQuery(CachedPlanSource *plansource)
/* OK, doesn't return tuples */
}
else if (resultDesc == NULL || plansource->resultDesc == NULL ||
- !equalTupleDescs(resultDesc, plansource->resultDesc))
+ !equalTupleDescs(resultDesc, plansource->resultDesc, false))
{
/* can we give a better error message? */
if (plansource->fixed_result)
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index 8d2ad01..228f1f8 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -2240,7 +2240,7 @@ RelationClearRelation(Relation relation, bool rebuild)
elog(ERROR, "relation %u deleted while still in use", save_relid);
}
- keep_tupdesc = equalTupleDescs(relation->rd_att, newrel->rd_att);
+ keep_tupdesc = equalTupleDescs(relation->rd_att, newrel->rd_att, true);
keep_rules = equalRuleLocks(relation->rd_rules, newrel->rd_rules);
keep_policies = equalRSDesc(relation->rd_rsdesc, newrel->rd_rsdesc);
diff --git a/src/backend/utils/cache/typcache.c b/src/backend/utils/cache/typcache.c
index ea6f787..b4578f5 100644
--- a/src/backend/utils/cache/typcache.c
+++ b/src/backend/utils/cache/typcache.c
@@ -1341,7 +1341,7 @@ assign_record_type_typmod(TupleDesc tupDesc)
foreach(l, recentry->tupdescs)
{
entDesc = (TupleDesc) lfirst(l);
- if (equalTupleDescs(tupDesc, entDesc))
Re: [HACKERS] Re: GiST optimizing memmoves in gistplacetopage for fixed-size updates [PoC]
>Can you please patch BRIN to use this new function too? AFAIK Sergey Mirvoda was going to implement and test it. It is easy to do this optimization for brin_samepage_update (see patch draft in attachment, make check passes), but WAL of regular BRIN update is not clear for me. Best regards, Andrey Borodin, Octonica & Ural Federal University. Brin_PageIndexTupleOverwrite.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Confusing TAP tests readme file
Hi all,
I was checking out TAP tests documentation. And I found confusing this
part of src/test/perl/README file:
my $ret = $node->psql('postgres', 'SELECT 1');
is($ret, '1', 'SELECT 1 returns 1');
The returning value of psql() function is the exit code of the psql.
Hence this test will never pass since psql returns 0 if query was
successfully executed. Probably it was meant to be the safe_psql()
function instead which returns stdout:
my $ret = $node->safe_psql('postgres', 'SELECT 1');
is($ret, '1', 'SELECT 1 returns 1');
or else:
my ($ret, $stdout, $stderr) =
$node->psql('postgres', 'SELECT 1');
is($stdout, '1', 'SELECT 1 returns 1');
The attached patch fixes this.
Regards,
Ildar Musin
--
Ildar Musin
i.mu...@postgrespro.ru
diff --git a/src/test/perl/README b/src/test/perl/README
index 9eae159..36d4120 100644
--- a/src/test/perl/README
+++ b/src/test/perl/README
@@ -41,7 +41,7 @@ against them and evaluate the results. For example:
$node->init;
$node->start;
-my $ret = $node->psql('postgres', 'SELECT 1');
+my $ret = $node->safe_psql('postgres', 'SELECT 1');
is($ret, '1', 'SELECT 1 returns 1');
$node->stop('fast');
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Optimizing numeric SUM() aggregate
Hi,
I spent some time profiling a simply query with a SUM() aggregate. We've
made some big improvements in this area in recent years, but it seems
there's still some room for improvement. A lot of CPU time is spent in
the numeric add_var() and friends. Which isn't that surprising, I guess.
I came up with the attached patch that keeps the sum in a specialized
accumulator, instead of a NumericVar. The specialized accumulator has a
few tricks, compared to the status quo:
1. Uses 32-bit integers to represent each base-1 "digit". Instead of
propagating carry after each new value, it's done only every values
(or at the end).
2. Accumulates positive and negative values separately. They positive
and negative sums are added together only at the end. This avoids the
overhead in add_var(), for figuring out which value is larger and
determining the result sign at each step.
3. Only allocate a new buffer when it needs to be enlarged. add_abs()
allocates a new one on every call.
These optimizations give a nice speedup for SUM(), and other aggregates
like AVG() and STDDEV() that use the same agg state. For example, using
the same test query that Hadi Moshayedi used on previous work on numeric
aggregates
(https://www.postgresql.org/message-id/CAK%3D1%3DWrmCkWc_xQXs_bpUyswCPr7O9zkLmm8Oa7_nT2vybvBEQ%40mail.gmail.com):
CREATE TABLE avg_test AS SELECT (random() * 999)::decimal(5,2) as d FROM
generate_series(1, 1000) s;
SELECT avg(d) FROM avg_test;
On my laptop, with max_parallel_workers_per_gather=0, this runs in about
1.5 s without the patch, and 1.2 s with the patch.
- Heikki
From 1f9556d13ca05dae4092e2c4a8a0d7b444039726 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas
Date: Mon, 25 Jul 2016 13:17:04 +0300
Subject: [PATCH 1/1] Speed up SUM calculation in numeric aggregates.
This introduces a numeric sum accumulator, which performs better than
repeatedly calling add_var(). The performance comes from using wider
digits and delaying carry propagation, and using separate sums for
positive and negative values, and avoiding a round of palloc/pfree on
every value. This speeds up SUM(), as well as other standard aggregates
like AVG() and STDDEV() that also calculate a sum internally.
---
src/backend/utils/adt/numeric.c | 596 +---
1 file changed, 492 insertions(+), 104 deletions(-)
diff --git a/src/backend/utils/adt/numeric.c b/src/backend/utils/adt/numeric.c
index 2fbdfe0..746a4c6 100644
--- a/src/backend/utils/adt/numeric.c
+++ b/src/backend/utils/adt/numeric.c
@@ -65,7 +65,8 @@
* and are no longer supported in any sense; no mechanism exists for the client
* to discover the base, so every client supporting binary mode expects the
* base-1 format. If you plan to change this, also note the numeric
- * abbreviation code, which assumes NBASE=1.
+ * abbreviation code, and the numeric sum accum code, which both assume
+ * NBASE=1.
* --
*/
@@ -302,6 +303,51 @@ typedef struct
hyperLogLogState abbr_card; /* cardinality estimator */
} NumericSortSupport;
+
+/* --
+ * Fast sum accumulator.
+ *
+ * NumericSumAccum is used to implement SUM(), and other standard aggregates
+ * that track the sum of input values. It uses 32-bit integers to store the
+ * digits, instead of the normal 16-bit integers (with NBASE=1). This
+ * way, we can safely accumulate up to values without propagating carry,
+ * before risking overflow any of the digits. Delaying carry propagation
+ * avoids a lot of overhead. 'num_uncarried' tracks how many values have been
+ * accumulated without propagating carry.
+ *
+ * Positive and negative values are accumulated separately, in 'pos_digits'
+ * 'neg_digits'. This is simpler and faster than deciding whether to add
+ * or subtract from the current value, for each new value (see sub_var()
+ * for the logic we avoid by doing this). Both buffers are of same size,
+ * and have the same weight and scale. In accum_sum_final(), the positive
+ * and negative sums are added together to produce the final result.
+ *
+ * When a new value has a larger ndigits or weight than the accumulator
+ * currently does, the ndigits and weight of the accumulator are enlarged
+ * to accommodate the new value. We normally have one zero digit reserved
+ * for carry propagation, and that is indicated by the 'have_carry_space'
+ * flag. When accum_sum_carry() uses up the reserved digit, it clears the
+ * 'have_carry_space' flag. The next call to accum_sum_add() will enlarge
+ * the buffer, to make room for the extra digit, and set the flag again.
+ *
+ * To initialize a new accumulator, simply reset all fields to zeros.
+ *
+ * The accumulator does not handle NaNs.
+ *
+ * --
+ */
+typedef struct NumericSumAccum
+{
+ int ndigits;
+ int weight;
+ int dscale;
+ int num_uncarried;
+ bool have_carry_space;
+ int32 *pos_digits;
+ int32 *neg_digits;
+} NumericSumAccum;
+
+
/*
* We define our
Re: [HACKERS] Curing plpgsql's memory leaks for statement-lifespan values
On Sun, Jul 24, 2016 at 9:17 PM, Tom Lane wrote: > Amit Kapila writes: > >> Wouldn't it be better, if each nested sub-block (which is having an >> exception) has a separate "SPI Proc", "SPI Exec" contexts which would >> be destroyed at sub-block end (either commit or rollback)? > > AFAICS that would just confuse matters. In the first place, plpgsql > variable values are not subtransaction-local, so they'd have to live in > the outermost SPI Proc context anyway. In the second place, spi.c > contains a whole lot of assumptions that actions like saving a plan are > tied to the current SPI Proc/Exec contexts, so SPI-using plpgsql > statements that were nested inside a BEGIN/EXCEPT would probably break: > state they expect to remain valid from one execution to the next would > disappear. > I think for all such usage, we can always switch to top level SPI Proc/Exec context. To do so, we might need to invent a notion of something like Sub SPI Proc/Exec contexts and that sounds quite tricky. >> In short, why do you think it is better to create a new context rather >> than using "SPI Exec"? > > "SPI Exec" has the same problem as the eval_econtext: there are already > points at which it will be reset, and those can't necessarily be delayed > till end of statement. In particular, _SPI_end_call will delete whatever > is in that context. > That's right and I think it might not be even feasible to do so, mainly because that is done in exposed SPI calls. I have checked some other usage of SPI, it seems plperl is handling some similar problems either via creating temporary work context or by using PG_TRY/PG_CATCH. I think it might be better if, whatever we are trying here could be of help for other similar usage. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Confusing TAP tests readme file
On Mon, Jul 25, 2016 at 7:42 PM, Ildar Musin wrote:
> I was checking out TAP tests documentation. And I found confusing this part
> of src/test/perl/README file:
>
> my $ret = $node->psql('postgres', 'SELECT 1');
> is($ret, '1', 'SELECT 1 returns 1');
Good catch.
> The returning value of psql() function is the exit code of the psql. Hence
> this test will never pass since psql returns 0 if query was successfully
> executed. Probably it was meant to be the safe_psql() function instead which
> returns stdout:
>
> my $ret = $node->safe_psql('postgres', 'SELECT 1');
> is($ret, '1', 'SELECT 1 returns 1');
>
> or else:
>
> my ($ret, $stdout, $stderr) =
> $node->psql('postgres', 'SELECT 1');
> is($stdout, '1', 'SELECT 1 returns 1');
>
> The attached patch fixes this.
Just using psql_safe looks fine to me.
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] sslmode=require fallback
On 7/20/16 8:55 AM, Daniel Verite wrote: > Personally I think that TLS for local networking is wrong as a default, and > it's unfortunate that distros like Debian/Ubuntu end up using that. There is something to that, but I'm not sure that just giving up and disabling SSL in the default configuration is a forward-looking answer. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Increasing timeout of poll_query_until for TAP tests
On Mon, Jul 25, 2016 at 2:52 PM, Michael Paquier
wrote:
> Ah, yes, and that's a stupid mistake. We had better use
> replay_location instead of write_location. There is a risk that
> records have not been replayed yet even if they have been written on
> the standby, so it is possible that the query looking at tab_int may
> not see this relation.
Or in short, the attached fixes 2) and will help providing input for 1)..
--
Michael
diff --git a/src/bin/pg_basebackup/t/010_pg_basebackup.pl b/src/bin/pg_basebackup/t/010_pg_basebackup.pl
index 6c33936..4b301d0 100644
--- a/src/bin/pg_basebackup/t/010_pg_basebackup.pl
+++ b/src/bin/pg_basebackup/t/010_pg_basebackup.pl
@@ -47,7 +47,7 @@ print CONF "wal_level = replica\n";
close CONF;
$node->restart;
-$node->command_ok([ 'pg_basebackup', '-D', "$tempdir/backup" ],
+$node->command_ok([ 'pg_basebackup', '--verbose', '-D', "$tempdir/backup" ],
'pg_basebackup runs');
ok(-f "$tempdir/backup/PG_VERSION", 'backup was created');
@@ -57,13 +57,14 @@ is_deeply(
'no WAL files copied');
$node->command_ok(
- [ 'pg_basebackup', '-D', "$tempdir/backup2", '--xlogdir',
+ [ 'pg_basebackup', '--verbose', '-D', "$tempdir/backup2", '--xlogdir',
"$tempdir/xlog2" ],
'separate xlog directory');
ok(-f "$tempdir/backup2/PG_VERSION", 'backup was created');
ok(-d "$tempdir/xlog2/", 'xlog directory was created');
-$node->command_ok([ 'pg_basebackup', '-D', "$tempdir/tarbackup", '-Ft' ],
+$node->command_ok(
+ [ 'pg_basebackup', '--verbose', '-D', "$tempdir/tarbackup", '-Ft' ],
'tar format');
ok(-f "$tempdir/tarbackup/base.tar", 'backup tar was created');
@@ -116,7 +117,8 @@ SKIP:
"CREATE TABLESPACE tblspc1 LOCATION '$shorter_tempdir/tblspc1';");
$node->safe_psql('postgres',
"CREATE TABLE test1 (a int) TABLESPACE tblspc1;");
- $node->command_ok([ 'pg_basebackup', '-D', "$tempdir/tarbackup2", '-Ft' ],
+ $node->command_ok(
+ [ 'pg_basebackup', '--verbose', '-D', "$tempdir/tarbackup2", '-Ft' ],
'tar format with tablespaces');
ok(-f "$tempdir/tarbackup2/base.tar", 'backup tar was created');
my @tblspc_tars = glob "$tempdir/tarbackup2/[0-9]*.tar";
@@ -127,7 +129,7 @@ SKIP:
'plain format with tablespaces fails without tablespace mapping');
$node->command_ok(
- [ 'pg_basebackup', '-D', "$tempdir/backup1", '-Fp',
+ [ 'pg_basebackup', '--verbose', '-D', "$tempdir/backup1", '-Fp',
"-T$shorter_tempdir/tblspc1=$tempdir/tbackup/tblspc1" ],
'plain format with tablespaces succeeds with tablespace mapping');
ok(-d "$tempdir/tbackup/tblspc1", 'tablespace was relocated');
@@ -146,7 +148,7 @@ SKIP:
$node->safe_psql('postgres',
"CREATE TABLESPACE tblspc2 LOCATION '$shorter_tempdir/tbl=spc2';");
$node->command_ok(
- [ 'pg_basebackup', '-D', "$tempdir/backup3", '-Fp',
+ [ 'pg_basebackup', '--verbose', '-D', "$tempdir/backup3", '-Fp',
"-T$shorter_tempdir/tbl\\=spc2=$tempdir/tbackup/tbl\\=spc2" ],
'mapping tablespace with = sign in path');
ok(-d "$tempdir/tbackup/tbl=spc2",
@@ -157,12 +159,13 @@ SKIP:
$node->safe_psql('postgres',
"CREATE TABLESPACE tblspc3 LOCATION '$tempdir/$superlongname';");
$node->command_ok(
- [ 'pg_basebackup', '-D', "$tempdir/tarbackup_l3", '-Ft' ],
+ [ 'pg_basebackup', '--verbose', '-D', "$tempdir/tarbackup_l3", '-Ft' ],
'pg_basebackup tar with long symlink target');
$node->safe_psql('postgres', "DROP TABLESPACE tblspc3;");
}
-$node->command_ok([ 'pg_basebackup', '-D', "$tempdir/backupR", '-R' ],
+$node->command_ok(
+ [ 'pg_basebackup', '--verbose', '-D', "$tempdir/backupR", '-R' ],
'pg_basebackup -R runs');
ok(-f "$tempdir/backupR/recovery.conf", 'recovery.conf was created');
my $recovery_conf = slurp_file "$tempdir/backupR/recovery.conf";
@@ -180,18 +183,19 @@ like(
'recovery.conf sets primary_conninfo');
$node->command_ok(
- [ 'pg_basebackup', '-D', "$tempdir/backupxf", '-X', 'fetch' ],
+ [ 'pg_basebackup', '--verbose', '-D', "$tempdir/backupxf", '-X', 'fetch' ],
'pg_basebackup -X fetch runs');
ok(grep(/^[0-9A-F]{24}$/, slurp_dir("$tempdir/backupxf/pg_xlog")),
'WAL files copied');
$node->command_ok(
- [ 'pg_basebackup', '-D', "$tempdir/backupxs", '-X', 'stream' ],
+ [ 'pg_basebackup', '--verbose', '-D', "$tempdir/backupxs", '-X',
+ 'stream' ],
'pg_basebackup -X stream runs');
ok(grep(/^[0-9A-F]{24}$/, slurp_dir("$tempdir/backupxf/pg_xlog")),
'WAL files copied');
$node->command_fails(
- [ 'pg_basebackup', '-D', "$tempdir/fail", '-S', 'slot1' ],
+ [ 'pg_basebackup', '--verbose', '-D', "$tempdir/fail", '-S', 'slot1' ],
'pg_basebackup with replication slot fails without -X stream');
$node->command_fails(
[ 'pg_basebackup', '-D',
@@ -207,8 +211,8 @@ my $lsn = $node->safe_psql('postgres',
);
is($lsn, '', 'restart LSN of new slot is null');
$node->command_ok(
- [ 'pg_basebackup', '-D', "$tempdir/backupxs_sl", '-X',
- 'stream','-S', 'slot1' ],
+ [ 'pg_basebackup', '--verbose', '-D', "$tempdir/backupxs_sl", '-X',
+ 'str
[HACKERS] No longer possible to query catalogs for index capabilities?
With the gutting of pg_am in 9.6, there seems to be no longer any way for a query of the system catalogs to discover any of the index capabilities that were formerly columns in pg_am (notably amcanorder, amcanorderbyop, amclusterable, amsearcharray, amsearchnulls). Am I missing something or is this a significant oversight? -- Andrew (irc:RhodiumToad) -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] No longer possible to query catalogs for index capabilities?
Andrew Gierth writes: > With the gutting of pg_am in 9.6, there seems to be no longer any way > for a query of the system catalogs to discover any of the index > capabilities that were formerly columns in pg_am (notably amcanorder, > amcanorderbyop, amclusterable, amsearcharray, amsearchnulls). > Am I missing something or is this a significant oversight? It's absolutely not an oversight. We asked when 65c5fcd35 went in whether there was still any need for that information to be available at the SQL level, and nobody appeared to care. We could in theory expose a view to show the data --- but since a large part of the point of that change was to not need initdb for AM API changes, and to not be constrained to exactly SQL-compatible representations within that API, I'm disinclined to do so without a fairly compelling argument why it's needed. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] No longer possible to query catalogs for index capabilities?
> "Tom" == Tom Lane writes: >> With the gutting of pg_am in 9.6, there seems to be no longer any >> way for a query of the system catalogs to discover any of the index >> capabilities that were formerly columns in pg_am (notably >> amcanorder, amcanorderbyop, amclusterable, amsearcharray, >> amsearchnulls). >> Am I missing something or is this a significant oversight? Tom> It's absolutely not an oversight. We asked when 65c5fcd35 went in Tom> whether there was still any need for that information to be Tom> available at the SQL level, and nobody appeared to care. Perhaps you were asking the wrong people? Tom> We could in theory expose a view to show the data --- but since a Tom> large part of the point of that change was to not need initdb for Tom> AM API changes, and to not be constrained to exactly Tom> SQL-compatible representations within that API, I'm disinclined to Tom> do so without a fairly compelling argument why it's needed. It could easily be exposed as a function interface of the form index_has_capability(oid,name) or indexam_has_capability(oid,name) without any initdb worries. That would surely be better than the present condition of being completely unable to get this information from SQL. -- Andrew (irc:RhodiumToad) -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] No longer possible to query catalogs for index capabilities?
* Andrew Gierth (and...@tao11.riddles.org.uk) wrote: > > "Tom" == Tom Lane writes: > > >> With the gutting of pg_am in 9.6, there seems to be no longer any > >> way for a query of the system catalogs to discover any of the index > >> capabilities that were formerly columns in pg_am (notably > >> amcanorder, amcanorderbyop, amclusterable, amsearcharray, > >> amsearchnulls). > > >> Am I missing something or is this a significant oversight? > > Tom> It's absolutely not an oversight. We asked when 65c5fcd35 went in > Tom> whether there was still any need for that information to be > Tom> available at the SQL level, and nobody appeared to care. > > Perhaps you were asking the wrong people? The capabilities strike me as useful to expose, they're pretty useful to know. I believe we were right to hide the APIs/functions and don't see any need to expose those to the SQL level. > Tom> We could in theory expose a view to show the data --- but since a > Tom> large part of the point of that change was to not need initdb for > Tom> AM API changes, and to not be constrained to exactly > Tom> SQL-compatible representations within that API, I'm disinclined to > Tom> do so without a fairly compelling argument why it's needed. > > It could easily be exposed as a function interface of the form > index_has_capability(oid,name) or indexam_has_capability(oid,name) > without any initdb worries. Hmm, that seems pretty reasonable. > That would surely be better than the present condition of being > completely unable to get this information from SQL. Agreed. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] AdvanceXLInsertBuffer vs. WAL segment compressibility
On 07/23/2016 08:25 AM, Amit Kapila wrote:
> On Sat, Jul 23, 2016 at 3:32 AM, Chapman Flack wrote:
>>
>> Would it then be possible to go back to the old behavior (or make
>> it selectable) of not overwriting the full 16 MB every time?
>>
>
> I don't see going back to old behaviour is an improvement, because as
> as you pointed out above that it helps to improve the compression
> ratio of WAL files for tools like gzip and it doesn't seem advisable
> to loose that capability. I think providing an option to select that
> behaviour could be one choice, but use case seems narrow to me
> considering there are tools (pglesslog) to clear the tail. Do you
> find any problems with that tool which makes you think that it is not
> reliable?
It was a year or so ago when I was surveying tools that attempted
to do that. I had found pg_clearxlogtail, and I'm sure I also found
pglesslog / pg_compresslog ... my notes from then simply refer to
"contrib efforts like pg_clearxlogtail" and observed either a dearth
of recent search results for them, or a predominance of results
of the form "how do I get this to compile for PG x.x?"
pg_compresslog is mentioned in a section, Compressed Archive Logs,
of the PG 9.1 manual:
https://www.postgresql.org/docs/9.1/static/continuous-archiving.html#COMPRESSED-ARCHIVE-LOGS
That section is absent in the docs any version > 9.1.
The impression that leaves is of tools that relied too heavily
on internal format knowledge to be viable outside of core, which
have had at least periods of incompatibility with newer PG versions,
and whose current status, if indeed any are current, isn't easy
to find out.
It seems a bit risky (to me, anyway) to base a backup strategy
on having a tool in the pipeline that depends so heavily on
internal format knowledge, can become uncompilable between PG
releases, and isn't part of core and officially supported.
And that, I assume, was also the motivation to put the zeroing
in AdvanceXLInsertBuffer, eliminating the need for one narrow,
specialized tool like pg{_clear,_compress,less}log{,tail}, so
the job can be done with ubiquitous, bog standard (and therefore
*very* exhaustively tested) tools like gzip.
So it's just kind of unfortunate that there used to be a *further*
factor of 100 (nothing to sneeze at) possible using rsync
(another non-PG-specific, ubiquitous, exhaustively tested tool)
but a trivial feature of the new behavior has broken that.
Factors of 100 are enough to change the sorts of things you think
about, like possibly retaining years-long unbroken histories of
transactions in WAL.
What would happen if the overwriting of the log tail were really
done with just zeros, as the git comment implied, rather than zeros
with initialized headers? Could the log-reading code handle that
gracefully? That would support all forms of non-PG-specific,
ubiquitous tools used for compression; it would not break the rsync
approach.
Even so, it still seems to me that a cheaper solution is a %e
substitution in archive_command: just *tell* the command where
the valid bytes end. Accomplishes the same thing as ~ 16 MB
of otherwise-unnecessary I/O at the time of archiving each
lightly-used segment.
Then the actual zeroing could be suppressed to save I/O, maybe
with a GUC variable, or maybe just when archive_command is seen
to contain a %e. Commands that don't have a %e continue to work
and compress effectively because of the zeroing.
-Chap
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] No longer possible to query catalogs for index capabilities?
Andrew Gierth writes: > "Tom" == Tom Lane writes: > Tom> We could in theory expose a view to show the data --- but since a > Tom> large part of the point of that change was to not need initdb for > Tom> AM API changes, and to not be constrained to exactly > Tom> SQL-compatible representations within that API, I'm disinclined to > Tom> do so without a fairly compelling argument why it's needed. > It could easily be exposed as a function interface of the form > index_has_capability(oid,name) or indexam_has_capability(oid,name) > without any initdb worries. You missed the "compelling argument why it's needed" part. What is the need for this? I'm not going to be persuaded by "it was there before". We've gotten along fine without such inspection functions for FDWs and tablesample methods, so I doubt that we really need them for index AMs. Nobody's writing applications that make decisions about which AM to use based on what they see in pg_am. And anyone who's concerned whether their AM is reporting the right info is going to be much better served by gdb than by some functions that can present only a subset of what's in the info struct. Moreover, I think you are missing the point about initdb. The issue there is that anytime in future that we make a change to the AM API, we'd need to have a debate about whether and how to expose such a change for SQL inspection. Defining the exposure mechanism as a new function rather than a new view column changes neither the need for a debate, nor the need for an initdb unless we decide that we don't need to expose anything. But if your proposal is merely that we freeze the set of information available as some subset of what used to be available from pg_am, then it sounds an awful lot like a backwards-compatibility hack rather than an honest attempt to describe AM capabilities. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] handling unconvertible error messages
Example: I have a database cluster initialized with --locale=ru_RU.UTF-8 (built with NLS). Let's say for some reason, I have client encoding set to LATIN1. All error messages come back like this: test=> select * from notthere; ERROR: character with byte sequence 0xd0 0x9e in encoding "UTF8" has no equivalent in encoding "LATIN1" There is no straightforward way for the client to learn that there is a real error message, but it could not be converted. I think ideally we could make this better in two ways: 1) Send the original error message untranslated. That would require saving the original error message in errmsg(), errdetail(), etc. That would be a lot of work for only the occasional use. But it would also facilitate an occasionally-requested feature of writing untranslated error messages into the server log or the csv log, while sending translated messages to the client (or some variant thereof). 2) Send an indication that there was an encoding problem. Maybe a NOTICE, or an error context? Wiring all this into elog.c looks a bit tricky, however. Ideas? -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] No longer possible to query catalogs for index capabilities?
> "Tom" == Tom Lane writes: >> It could easily be exposed as a function interface of the form >> index_has_capability(oid,name) or indexam_has_capability(oid,name) >> without any initdb worries. Tom> You missed the "compelling argument why it's needed" part. What Tom> is the need for this? I'm not going to be persuaded by "it was Tom> there before". How about "it was there before, and people did use it"? In fact I notice you participated in a discussion of this a couple of months back on the JDBC list, in which your solution was to suggest hardcoding the name 'btree' into the query: https://www.postgresql.org/message-id/24504.1463237368%40sss.pgh.pa.us Doesn't that strike you as an indication that something is wrong? Tom> We've gotten along fine without such inspection functions for FDWs Tom> and tablesample methods, which are new and not especially interesting to code doing introspection Tom> so I doubt that we really need them for index AMs. People write catalog queries for indexes a whole lot more than they do for FDWs or tablesample methods. This whole discussion started because I wrote a catalog query for someone on IRC, and found I couldn't do it on 9.6 because amcanorder was gone. Tom> Nobody's writing applications that make decisions about which AM Tom> to use based on what they see in pg_am. That's not the issue. The issue is finding information about _existing_ indexes that is not otherwise exposed. Tom> Moreover, I think you are missing the point about initdb. The Tom> issue there is that anytime in future that we make a change to the Tom> AM API, we'd need to have a debate about whether and how to expose Tom> such a change for SQL inspection. Defining the exposure mechanism Tom> as a new function rather than a new view column changes neither Tom> the need for a debate, nor the need for an initdb unless we decide Tom> that we don't need to expose anything. I'm not proposing a new function for each capability. I'm proposing ONE function (or two, one starting from the index rather than the AM, for convenience). Adding more capability names would not require an initdb. -- Andrew (irc:RhodiumToad) -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] ispell/hunspell imprecision in error message
I was trying to look up the background of this error message: "Ispell dictionary supports only \"default\", \"long\", and \"num\" flag value" (src/backend/tsearch/spell.c) But I found that this actually talks about Hunspell format dictionaries. (So the man page is hunspell(5) as opposed to ispell(5).) While as far as the tsearch interfaces are concerned, those two are lumped together, should we not change the error message to refer to Hunspell? -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] No longer possible to query catalogs for index capabilities?
On Mon, Jul 25, 2016 at 10:35 AM, Tom Lane wrote: > You missed the "compelling argument why it's needed" part. What is the > need for this? It's self-evident that this thread wouldn't exist if it were not the case that people had queries that no longer work because of these new changes. You can hold your breath and pretend that every single one of those queries is probably misdesigned, but I do not think anyone else will find that argument convincing. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] LWLocks in DSM memory
On Sun, Jul 24, 2016 at 11:02 PM, Thomas Munro
wrote:
> On Sun, Jul 24, 2016 at 1:10 AM, Thomas Munro
> wrote:
>> One solution could be to provide a non-circular variant of the dlist
>> interface that uses NULL list termination. I've attached a quick
>> sketch of something like that which seems to work correctly. It is
>> only lightly tested so far and probably buggy, but shows the general
>> idea.
>
> On reflection, it wouldn't make much sense to put "noncircular" in the
> names of interfaces, as that is an internal detail. Maybe
> "relocatable" or "position independent" or something like that, since
> that's a user-visible property of the dlist_head. Here is a version
> of the patch that uses _relocatable.
>
>> Any thoughts on this approach, or better ways to solve this problem?
>> How valuable is the branch-free property of the circular push and
>> delete operations?
>
> I investigated this a bit. If I do this on my laptop (clang, no
> asserts, -O2), it takes 3895 milliseconds, or 4.8ns per push/delete
> operation:
>
> dlist_init(&head);
> for (i = 0; i < 1; ++i)
> {
> dlist_push_head(&head, &nodes[0]);
> dlist_push_tail(&head, &nodes[1]);
> dlist_push_head(&head, &nodes[2]);
> dlist_push_tail(&head, &nodes[3]);
> dlist_delete(&nodes[2]);
> dlist_delete(&nodes[3]);
> dlist_delete(&nodes[0]);
> dlist_delete(&nodes[1]);
> }
>
> The relocatable version takes 5907 milliseconds, or 7.4ns per
> push/delete operation:
>
> dlist_init_relocatable(&head);
> for (i = 0; i < 1; ++i)
> {
> dlist_push_head_relocatable(&head, &nodes[0]);
> dlist_push_tail_relocatable(&head, &nodes[1]);
> dlist_push_head_relocatable(&head, &nodes[2]);
> dlist_push_tail_relocatable(&head, &nodes[3]);
> dlist_delete_relocatable(&head, &nodes[2]);
> dlist_delete_relocatable(&head, &nodes[3]);
> dlist_delete_relocatable(&head, &nodes[0]);
> dlist_delete_relocatable(&head, &nodes[1]);
> }
>
> Those operations are ~50% slower. So getting rid of dlist's clever
> branch-free code generally seems like a bad idea.
>
> Next I wondered if it would be a bad idea to use slower relocatable
> dlist heads for all LWLocks. Obviously LWLocks are used all over the
> place and it would be bad to slow them down, but I wondered if maybe
> dlist manipulation might not be a very significant part of their work.
> So I put a LWLock and a counter in shared memory, and had N background
> workers run a tight loop that locks, increments and unlocks
> concurrently until the counter reaches 1 billion. This creates
> different degrees of contention and wait list sizes. The worker loop
> looks like this:
>
> while (!finished)
> {
> LWLockAcquire(&control->lock, LW_EXCLUSIVE);
> ++control->counter;
> if (control->counter >= control->goal)
> finished = true;
> LWLockRelease(&control->lock);
> }
>
> I measured the following times for unpatched master, on my 4 core laptop:
>
> 16 workers = 73.067s, 74.869s, 75.338s
> 8 workers = 65.846s, 67.622s, 68.039s
> 4 workers = 68.763s, 68.980s, 69.035s <-- curiously slower here
> 3 workers = 59.701s, 59.991s, 60.133s
> 2 workers = 53.620s, 55.300s, 55.790s
> 1 worker = 21.578s, 21.535s, 21.598s
>
> With the attached patched I got:
>
> 16 workers = 75.341s, 77.445s, 77.635s <- +3.4%
> 8 workers = 67.462s, 68.622s, 68.851s <- +1.4%
> 4 workers = 64.983s, 65.021s, 65.496s <- -5.7%
> 3 workers = 60.247s, 60.425s, 60.492s <- +0.7%
> 2 workers = 57.544s, 57.626s, 58.133s <- +2.3%
> 1 worker = 21.403s, 21.486s, 21.661s <- -0.2%
>
> Somewhat noisy data and different effects at different numbers of workers.
>
> I can post the source for those tests if anyone is interested. If you
> have any other ideas for access patterns to test, or clever ways to
> keep push and delete branch-free while also avoiding internal pointers
> back to dlist_head, I'm all ears. Otherwise, if a change affecting
> all LWLocks turns out to be unacceptable, maybe we would need to have
> a different LWLock interface for relocatable LWLocks to make them
> suitable for use in DSM segments. Any thoughts?
Thanks for looking into this. Any theory on why this is slower in the
abstract but not slower, perhaps even faster, for LWLocks? I wonder
if it has to do with how likely it is for a list to be completely
empty.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] LWLocks in DSM memory
Hi,
On 2016-07-25 15:02:41 +1200, Thomas Munro wrote:
> > Any thoughts on this approach, or better ways to solve this problem?
> > How valuable is the branch-free property of the circular push and
> > delete operations?
I think it's generally valuable, but I suspect that it doesn't really
matter much for lwlocks, given that this is already the slow path, where
we enter the kernel and such. I suspect that the performance difference
here is a more due to code movement than anything.
> Next I wondered if it would be a bad idea to use slower relocatable
> dlist heads for all LWLocks. Obviously LWLocks are used all over the
> place and it would be bad to slow them down, but I wondered if maybe
> dlist manipulation might not be a very significant part of their work.
I'm pretty sure thats's the case.
> So I put a LWLock and a counter in shared memory, and had N background
> workers run a tight loop that locks, increments and unlocks
> concurrently until the counter reaches 1 billion. This creates
> different degrees of contention and wait list sizes. The worker loop
> looks like this:
>
> while (!finished)
> {
> LWLockAcquire(&control->lock, LW_EXCLUSIVE);
> ++control->counter;
> if (control->counter >= control->goal)
> finished = true;
> LWLockRelease(&control->lock);
> }
You really need shared lwlocks here as well, because exclusive lwlocks
will only ever trigger a single list manipulation (basically a pop from
the wait queue), further minimizing the list manipulation overhead.
I think the better fix here would acutally be to get rid of a pointer
based list here, and a) replace the queue with integer offsets b) make
the queue lock-free. But I understand that you might not want to tackle
that :P
Greetings,
Andres Freund
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Curing plpgsql's memory leaks for statement-lifespan values
Attached is a draft patch for creating statement-level temporary memory
contexts in plpgsql. It creates such contexts only as needed, and there
are a lot of simpler plpgsql statements that don't need them, so I think
the performance impact should be pretty minimal --- though I've not tried
to measure it, since I don't have a credible benchmark for overall plpgsql
performance.
This fixes the originally complained-of leak and several others besides,
including at least one path that leaked function-lifespan memory even
without an error :-(.
In addition to the patch proper, I attach for amusement's sake some
additional hacking I did for testing purposes, to make sure I'd found and
accounted for all the places that were allocating memory in the SPI Proc
context. There's a glibc-dependent hack in aset.c that reports any
plpgsql-driven palloc or pfree against a context named "SPI Proc", as
well as changes in pl_comp.c so that transient junk created during initial
parsing of a plpgsql function body doesn't end up in the SPI Proc context.
(I did that just to cut the amount of noise I had to chase down. I suppose
in large functions it might be worth adopting such a change so that that
junk can be released immediately after parsing; but I suspect for most
functions it'd just be an extra context without much gain.)
Although this is in principle a bug fix, it's invasive enough that I'd
be pretty hesitant to back-patch it, or even to stick it into HEAD
post-beta. I'm inclined to sign it up for the next commitfest instead.
regards, tom lane
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index 586ff1f..e23ca96 100644
*** a/src/pl/plpgsql/src/pl_exec.c
--- b/src/pl/plpgsql/src/pl_exec.c
*** typedef struct
*** 48,54
Oid *types; /* types of arguments */
Datum *values; /* evaluated argument values */
char *nulls; /* null markers (' '/'n' style) */
- bool *freevals; /* which arguments are pfree-able */
} PreparedParamsData;
/*
--- 48,53
*** static EState *shared_simple_eval_estate
*** 88,93
--- 87,122
static SimpleEcontextStackEntry *simple_econtext_stack = NULL;
/*
+ * Memory management within a plpgsql function generally works with three
+ * contexts:
+ *
+ * 1. Function-call-lifespan data, such as variable values, is kept in the
+ * "main" context, a/k/a the "SPI Proc" context established by SPI_connect().
+ * This is usually the CurrentMemoryContext while running code in this module
+ * (which is not good, because careless coding can easily cause
+ * function-lifespan memory leaks, but we live with it for now).
+ *
+ * 2. Some statement-execution routines need statement-lifespan workspace.
+ * A suitable context is created on-demand by get_stmt_mcontext(), and must
+ * be reset at the end of the requesting routine. Error recovery will clean
+ * it up automatically. Nested statements requiring statement-lifespan
+ * workspace will result in a stack of such contexts, see push_stmt_mcontext().
+ *
+ * 3. We use the eval_econtext's per-tuple memory context for expression
+ * evaluation, and as a general-purpose workspace for short-lived allocations.
+ * Such allocations usually aren't explicitly freed, but are left to be
+ * cleaned up by a context reset, typically done by exec_eval_cleanup().
+ *
+ * These macros are for use in making short-lived allocations:
+ */
+ #define get_eval_mcontext(estate) \
+ ((estate)->eval_econtext->ecxt_per_tuple_memory)
+ #define eval_mcontext_alloc(estate, sz) \
+ MemoryContextAlloc(get_eval_mcontext(estate), sz)
+ #define eval_mcontext_alloc0(estate, sz) \
+ MemoryContextAllocZero(get_eval_mcontext(estate), sz)
+
+ /*
* We use a session-wide hash table for caching cast information.
*
* Once built, the compiled expression trees (cast_expr fields) survive for
*** static HTAB *shared_cast_hash = NULL;
*** 128,133
--- 157,165
/
static void plpgsql_exec_error_callback(void *arg);
static PLpgSQL_datum *copy_plpgsql_datum(PLpgSQL_datum *datum);
+ static MemoryContext get_stmt_mcontext(PLpgSQL_execstate *estate);
+ static void push_stmt_mcontext(PLpgSQL_execstate *estate);
+ static void pop_stmt_mcontext(PLpgSQL_execstate *estate);
static int exec_stmt_block(PLpgSQL_execstate *estate,
PLpgSQL_stmt_block *block);
*** static void exec_eval_cleanup(PLpgSQL_ex
*** 191,197
static void exec_prepare_plan(PLpgSQL_execstate *estate,
PLpgSQL_expr *expr, int cursorOptions);
static bool exec_simple_check_node(Node *node);
! static void exec_simple_check_plan(PLpgSQL_expr *expr);
static void exec_simple_recheck_plan(PLpgSQL_expr *expr, CachedPlan *cplan);
static void exec_check_rw_parameter(PLpgSQL_expr *expr, int target_dno);
static bool contains_target_param(Node *node, int *target_dno);
--- 223,22
Re: [HACKERS] No longer possible to query catalogs for index capabilities?
Robert Haas writes: > On Mon, Jul 25, 2016 at 10:35 AM, Tom Lane wrote: >> You missed the "compelling argument why it's needed" part. What is the >> need for this? > It's self-evident that this thread wouldn't exist if it were not the > case that people had queries that no longer work because of these new > changes. You can hold your breath and pretend that every single one > of those queries is probably misdesigned, but I do not think anyone > else will find that argument convincing. We've already broken existing queries against pg_am, simply because the columns are not there anymore; and that decision is not getting undone at this point. I'm willing to consider putting back some substitute capability, but I'd like to see as much evidence for adding that as we'd expect for any other new feature. Andrew still hasn't shown a concrete example of what he needs to do and why. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] No longer possible to query catalogs for index capabilities?
Here is my proposed code (first cut; obviously it needs docs too).
Opinions?
--
Andrew (irc:RhodiumToad)
diff --git a/src/backend/access/index/amapi.c b/src/backend/access/index/amapi.c
index d347ebc..3e7e084 100644
--- a/src/backend/access/index/amapi.c
+++ b/src/backend/access/index/amapi.c
@@ -16,7 +16,9 @@
#include "access/amapi.h"
#include "access/htup_details.h"
#include "catalog/pg_am.h"
+#include "catalog/pg_class.h"
#include "catalog/pg_opclass.h"
+#include "utils/elog.h"
#include "utils/syscache.h"
@@ -119,3 +121,95 @@ amvalidate(PG_FUNCTION_ARGS)
PG_RETURN_BOOL(result);
}
+
+
+/*
+ * Test capabilities of an index or index AM.
+ */
+static Datum
+indexam_capability(FunctionCallInfo fcinfo,
+ Oid amoid, char *nameptr, int namelen)
+{
+ IndexAmRoutine *routine = NULL;
+ bool ret;
+
+ if (namelen < 1 || namelen > 14)
+ PG_RETURN_NULL();
+
+ routine = GetIndexAmRoutineByAmId(amoid);
+
+ if (namelen == 10 && memcmp(nameptr, "amcanorder", 10) == 0)
+ ret = (routine->amcanorder) ? true : false;
+ else if (namelen == 14 && memcmp(nameptr, "amcanorderbyop", 14) == 0)
+ ret = (routine->amcanorderbyop) ? true : false;
+ else if (namelen == 13 && memcmp(nameptr, "amcanbackward", 13) == 0)
+ ret = (routine->amcanbackward) ? true : false;
+ else if (namelen == 11 && memcmp(nameptr, "amcanunique", 11) == 0)
+ ret = (routine->amcanunique) ? true : false;
+ else if (namelen == 13 && memcmp(nameptr, "amcanmulticol", 13) == 0)
+ ret = (routine->amcanmulticol) ? true : false;
+ else if (namelen == 13 && memcmp(nameptr, "amoptionalkey", 13) == 0)
+ ret = (routine->amoptionalkey) ? true : false;
+ else if (namelen == 13 && memcmp(nameptr, "amsearcharray", 13) == 0)
+ ret = (routine->amsearcharray) ? true : false;
+ else if (namelen == 13 && memcmp(nameptr, "amsearchnulls", 13) == 0)
+ ret = (routine->amsearchnulls) ? true : false;
+ else if (namelen == 9 && memcmp(nameptr, "amstorage", 9) == 0)
+ ret = (routine->amstorage) ? true : false;
+ else if (namelen == 13 && memcmp(nameptr, "amclusterable", 13) == 0)
+ ret = (routine->amclusterable) ? true : false;
+ else if (namelen == 11 && memcmp(nameptr, "ampredlocks", 11) == 0)
+ ret = (routine->ampredlocks) ? true : false;
+ else if (namelen == 11 && memcmp(nameptr, "amcanreturn", 11) == 0)
+ ret = (routine->amcanreturn) ? true : false;
+ else if (namelen == 10 && memcmp(nameptr, "amgettuple", 10) == 0)
+ ret = (routine->amgettuple) ? true : false;
+ else if (namelen == 11 && memcmp(nameptr, "amgetbitmap", 11) == 0)
+ ret = (routine->amgetbitmap) ? true : false;
+ else
+ PG_RETURN_NULL();
+
+ PG_RETURN_BOOL(ret);
+}
+
+/*
+ * Test capability of an AM specified by the AM Oid.
+ */
+Datum
+pg_indexam_has_capability(PG_FUNCTION_ARGS)
+{
+ Oid amoid = PG_GETARG_OID(0);
+ text *name = PG_GETARG_TEXT_PP(1);
+ char *nameptr = VARDATA_ANY(name);
+ int namelen = VARSIZE_ANY_EXHDR(name);
+
+ return indexam_capability(fcinfo, amoid, nameptr, namelen);
+}
+
+/*
+ * Test capability of the AM for an index specified by relation Oid.
+ */
+Datum
+pg_index_has_capability(PG_FUNCTION_ARGS)
+{
+ Oid relid = PG_GETARG_OID(0);
+ text *name = PG_GETARG_TEXT_PP(1);
+ char *nameptr = VARDATA_ANY(name);
+ int namelen = VARSIZE_ANY_EXHDR(name);
+ Oid amoid;
+ HeapTuple tuple;
+ Form_pg_class rd_rel;
+
+ tuple = SearchSysCache1(RELOID, ObjectIdGetDatum(relid));
+ if (!HeapTupleIsValid(tuple))
+ elog(ERROR, "could not find tuple for relation %u", relid);
+ rd_rel = (Form_pg_class) GETSTRUCT(tuple);
+ if (rd_rel->relkind != RELKIND_INDEX)
+ ereport(ERROR,
+(errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg("relation %u is not an index", relid)));
+ amoid = rd_rel->relam;
+ ReleaseSysCache(tuple);
+
+ return indexam_capability(fcinfo, amoid, nameptr, namelen);
+}
diff --git a/src/include/access/amapi.h b/src/include/access/amapi.h
index 35f1061..0e2f9d7 100644
--- a/src/include/access/amapi.h
+++ b/src/include/access/amapi.h
@@ -171,4 +171,7 @@ extern IndexAmRoutine *GetIndexAmRoutineByAmId(Oid amoid);
extern Datum amvalidate(PG_FUNCTION_ARGS);
+extern Datum pg_indexam_has_capability(PG_FUNCTION_ARGS);
+extern Datum pg_index_has_capability(PG_FUNCTION_ARGS);
+
#endif /* AMAPI_H */
diff --git a/src/include/catalog/pg_proc.h b/src/include/catalog/pg_proc.h
index 5d233e3..4e696fa 100644
--- a/src/include/catalog/pg_proc.h
+++ b/src/include/catalog/pg_proc.h
@@ -564,6 +564,11 @@ DESCR("spgist index access method handler");
DATA(insert OID = 335 ( brinhandler PGNSP PGUID 12 1 0 0 0 f f f f t f v s 1 0 325 "2281" _null_ _null_ _null_ _null_ _null_ brinhandler _null_ _null_ _null_ ));
DESCR("brin index access method handler");
+DATA(insert OID = 336 ( pg_indexam_has_capability PGNSP PGUID 12 1 0 0 0 f f f f t f s s 2 0 16 "26 25" _null_ _null_ _null_ _null_ _null_ pg_indexam_has_capability _null_ _null_ _null_ ));
+DESCR("test capability of an index access method");
+DATA(insert OID = 337 ( pg_index_has_capability PGNSP PGUID
Re: [HACKERS] No longer possible to query catalogs for index capabilities?
> "Tom" == Tom Lane writes: Tom> Andrew still hasn't shown a concrete example of what he needs to Tom> do and why. The issue I ran into was the exact same one as in the JDBC thread I linked to earlier: correctly interpreting pg_index.indoption (to get the ASC / DESC and NULLS FIRST/LAST settings), which requires knowing whether amcanorder is true to determine whether to look at the bits at all. The guy I was helping was using an earlier pg version, so it didn't affect him (yet); I hit it when trying to test the query on 9.6. -- Andrew (irc:RhodiumToad) -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] No longer possible to query catalogs for index capabilities?
On 07/25/2016 12:19 PM, Tom Lane wrote: Robert Haas writes: On Mon, Jul 25, 2016 at 10:35 AM, Tom Lane wrote: You missed the "compelling argument why it's needed" part. What is the need for this? It's self-evident that this thread wouldn't exist if it were not the case that people had queries that no longer work because of these new changes. You can hold your breath and pretend that every single one of those queries is probably misdesigned, but I do not think anyone else will find that argument convincing. We've already broken existing queries against pg_am, simply because the columns are not there anymore; and that decision is not getting undone at this point. I'm willing to consider putting back some substitute capability, but I'd like to see as much evidence for adding that as we'd expect for any other new feature. Andrew still hasn't shown a concrete example of what he needs to do and why. I think that Andrew and other people who have commented on this thread made it pretty obvious why it is useful. JD regards, tom lane -- Command Prompt, Inc. http://the.postgres.company/ +1-503-667-4564 PostgreSQL Centered full stack support, consulting and development. Everyone appreciates your honesty, until you are honest with them. Unless otherwise stated, opinions are my own. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] No longer possible to query catalogs for index capabilities?
"Joshua D. Drake" writes: > On 07/25/2016 12:19 PM, Tom Lane wrote: >> Andrew still hasn't shown a concrete >> example of what he needs to do and why. > I think that Andrew and other people who have commented on this thread > made it pretty obvious why it is useful. Both Andrew and Robert have asserted without proof that it'd be useful to be able to get at some of that data. Given the lack of any supporting evidence, it's impossible to know which data needs to be exposed, and that's why I find their statements insufficient. "Emulate 9.5's pg_am exactly" is not in the cards, and short of that I'd like to see some concrete reasons why we do or do not need to expose particular bits of data. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH v12] GSSAPI encryption support
Robbie Harwood writes: > Michael Paquier writes: > >> On Thu, Apr 7, 2016 at 8:20 AM, Tom Lane wrote: >>> Robbie Harwood writes: Tom Lane writes: > Wait a second. So the initial connection-request packet is > necessarily unencrypted under this scheme? >>> Yes, by necessity. The username must be sent in the clear, even if only as part of the GSSAPI handshake (i.e., the GSSAPI username will appear in plantext in the GSSAPI blobs which are otherwise encrypted). GSSAPI performs authentication before it can start encryption. >>> >>> Ugh. I had thought we were putting work into this because it >>> represented something we could recommend as best practice, but now >>> you're telling me that it's always going to be inferior to what we >>> have already. >> >> It does not seem necessary to have an equivalent of >> pqsecure_open_client, just some extra handling in fe-connect.c to set >> up the initial context with a proper message handling... Not that >> direct anyway. So should the patch be marked as returned with feedback >> at this stage? > > I think in order to satisfy Tom's (valid) concern, there does need to be > a separate handshake - i.e., GSSAPI support in pqsecure_open_client(). > > If I were to continue as I have been - using the plaintext connection > and auth negotiation path - then at the time of startup the client has > no way of knowing whether to send connection parameters or not. > Personally, I would be in favor of not frontloading these connection > parameters over insecure connections, but it is my impression that the > project does not want to go this way (which is fine). > > The way I'm seeing this, when a connection comes in, we take the 'G' > character for GSSAPI much as for SSL. At that time, we need to perform > an *authentication* handshake (because GSSAPI will not do encryption > before authenticating). I expect to use a consistent format for all > GSSAPI packets - four bytes for length, and a payload. (I would prefer > tagging them, but previously preference for not doing this has been > expressed.) > > Once GSSAPI authentication is complete, the normal handshake process can > be tunneled through a GSSAPI encryption layer, as is done with TLS. The > server will need to retain some of the earlier authentication data > (e.g., to check that the presented user-name matches GSSAPI > credentials), but there will be no authentication packets exchanged > (more specifically, it will resemble the anonymous case). Authorization > will be checked as normal, and we then proceed in the usual fashion, all > over the GSSAPI tunnel. > > On the server, I'll need to implement `hostgss` (by analogy to > `hostssl`), and we'll want to lock authentication on those connections > to GSSAPI-only. Clients will explicitly probe for GSSAPI support as > they do for TLS support (I look forward to the bikeshed on the order of > these) and should have a parameter to require said support. One thing > I'm not clear on is what our behavior should be when the user doesn't > explicitly request GSSAPI and doesn't have a ccache - do we prompt? > Skip probing? I'm not sure what the best option there is. > > Before I implement this design, does anyone have any additional concerns > or feedback on it? Does this look reasonable to folks? signature.asc Description: PGP signature
Re: [HACKERS] No longer possible to query catalogs for index capabilities?
* Tom Lane (t...@sss.pgh.pa.us) wrote: > "Joshua D. Drake" writes: > > On 07/25/2016 12:19 PM, Tom Lane wrote: > >> Andrew still hasn't shown a concrete > >> example of what he needs to do and why. > > > I think that Andrew and other people who have commented on this thread > > made it pretty obvious why it is useful. > > Both Andrew and Robert have asserted without proof that it'd be useful > to be able to get at some of that data. Given the lack of any supporting > evidence, it's impossible to know which data needs to be exposed, and > that's why I find their statements insufficient. "Emulate 9.5's pg_am > exactly" is not in the cards, and short of that I'd like to see some > concrete reasons why we do or do not need to expose particular bits of > data. I believe the response to "what" is the patch which Andrew provided, and the use-case is illustrated by the query which he wrote that used those columns in much the same way that the JDBC driver used them (and which was also broken by their removal). This isn't just academic "gee, I wish we hadn't removed those columns", there are clearly cases where they were useful and were used. I do not believe hard-coding the name of index types as a definitive list of which indexes support what capabilities is an appropriate approach (as was suggested, and evidently done, for the JDBC driver). Additional use-cases include query analysis, by which one might want to see what capabilities an index has to understand why it may or may not be useful for a given query. I would also suggest that relying on pg_get_indexdef() is a poor solution and we should be considering how to expose the necessary information for pg_dump through the catalog instead of asking users who are interested to use a function that returns the result as an SQL DDL statement. We don't do that for table definitions and have argued time and time again why we shouldn't. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Curing plpgsql's memory leaks for statement-lifespan values
Tom Lane wrote: > Although this is in principle a bug fix, it's invasive enough that I'd > be pretty hesitant to back-patch it, or even to stick it into HEAD > post-beta. I'm inclined to sign it up for the next commitfest instead. Do we have a backpatchable fix for the reported problem? If so, then it seems a good tradeoff to install this more invasive fix in HEAD only and apply the simpler fix to back branches. Otherwise, is the proposal to leave the bug unfixed in back branches? -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Curing plpgsql's memory leaks for statement-lifespan values
Alvaro Herrera wrote: > Tom Lane wrote: > > > Although this is in principle a bug fix, it's invasive enough that I'd > > be pretty hesitant to back-patch it, or even to stick it into HEAD > > post-beta. I'm inclined to sign it up for the next commitfest instead. > > Do we have a backpatchable fix for the reported problem? If so, then it > seems a good tradeoff to install this more invasive fix in HEAD only and > apply the simpler fix to back branches. Otherwise, is the proposal to > leave the bug unfixed in back branches? I meant "install in HEAD only, after the 10.0 branch", which is what you proposed. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Curing plpgsql's memory leaks for statement-lifespan values
Alvaro Herrera writes: > Tom Lane wrote: >> Although this is in principle a bug fix, it's invasive enough that I'd >> be pretty hesitant to back-patch it, or even to stick it into HEAD >> post-beta. I'm inclined to sign it up for the next commitfest instead. > Do we have a backpatchable fix for the reported problem? If so, then it > seems a good tradeoff to install this more invasive fix in HEAD only and > apply the simpler fix to back branches. Otherwise, is the proposal to > leave the bug unfixed in back branches? The latter is what I was thinking. Given that issues like this have been there, and gone unreported, since we invented subtransactions, I do not feel too awful about not fixing it in existing branches. It's possible that we could fix just the one issue originally complained of with a smaller patch, but I don't find that approach attractive, because it was also a small leak. The case that seemed nastiest to me is that a FOREACH ... IN ARRAY loop will leak a copy of the entire array if an error causes control to be thrown out of exec_stmt_foreach_a. That could be a lot more leaked data than the querystring of an EXECUTE. I suppose that a fix based on putting PG_TRY blocks into all the affected functions might be simple enough that we'd risk back-patching it, but I don't really want to go that way. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Design for In-Core Logical Replication
On 07/20/2016 10:08 AM, Simon Riggs wrote: > > Monitoring > > pg_stat_replication > > > pg_stat_subscription > > and probably also `pg_stat_publication` or some other way to see, what tables are currently in a PUBLICATION, who has subscribed etc. > > CREATE PUBLICATION mypub; > ALTER PUBLICATION mypub ADD TABLE users, departments; > Would a subscription just be a logical grouping or would it be something stronger like meaning atomic subscriptions and/or a dedicated replication slot ? Can you subscribe to multiple publications through single SUBSCRIPTION ? What is supposed to happen if table A is in two subscriptions S1 and S2, and you subscribe to both? Will you get table a only once (both initial copy and events)? Would a subscription of "mypub" pop up on subscriber side atomically, or will subscribed tables appear one-by one when they are ready (initial copy + catchup event replay completed) ? I recall that one of the drivers of developing pgq/skytools to replace Slony was the fact that Slony's "replication group" design made it very easy to blunder subscription changes in more complex topologies which manifested in deadlocks. PGQ-s table-by-table subscription avoided this entirely at the cost on non-atomic subscribed table appearance. Of course once subscribed, everything was transaction-consistent again. > > CREATE SUBSCRIPTION mysub WITH CONNECTION dbname=foo host=bar > user=repuser PUBLICATION mypub; > For the pgq-like version which consider a PUBLICATION just as list of tables to subscribe, I would add CREATE SUBSCRIPTION mysub WITH CONNECTION 'dbname=foo host=bar user=repuser' PUBLICATION mypub, mypub1; ALTER SUBSCRIPTION mysub DROP PUBLICATION mypub1; ALTER SUBSCRIPTION mysub ADD PUBLICATION mypub2; -- Hannu Krosing PostgreSQL Consultant Performance, Scalability and High Availability 2ndQuadrant Nordic OÜ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] No longer possible to query catalogs for index capabilities?
On 25/07/16 21:20, Andrew Gierth wrote: > Here is my proposed code (first cut; obviously it needs docs too). > Opinions? I support the future of this patch, for 9.6. -- Vik Fearing +33 6 46 75 15 36 http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proposal: revert behavior of IS NULL on row types
On 7/22/16 7:01 PM, Andrew Gierth wrote: > In light of the fact that it is an endless cause of bugs both in pg and > potentially to applications, I propose that we cease attempting to > conform to the spec's definition of IS NULL in favour of the following > rules: I can't see how we would incompatibly change existing standards-conforming behavior merely because users are occasionally confused and there are some bugs in corner cases. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH v12] GSSAPI encryption support
On Tue, Jul 26, 2016 at 5:58 AM, Robbie Harwood wrote: > Robbie Harwood writes: Sorry for my late reply. >> I think in order to satisfy Tom's (valid) concern, there does need to be >> a separate handshake - i.e., GSSAPI support in pqsecure_open_client(). Having the communication layer in fe-secure.c definitely makes sense. The startup process though should not use CONNECTION_SSL_STARTUP. >> If I were to continue as I have been - using the plaintext connection >> and auth negotiation path - then at the time of startup the client has >> no way of knowing whether to send connection parameters or not. >> Personally, I would be in favor of not frontloading these connection >> parameters over insecure connections, but it is my impression that the >> project does not want to go this way (which is fine). Per the discussion upthread I got the opposite impression, the startup packet should be sent after the connection has been established. SSL does so: the SSL negotiation goes first, and then the startup packet is sent. That's the flow with the status changing from CONNECTION_SSL_START -> CONNECTION_MADE. >> The way I'm seeing this, when a connection comes in, we take the 'G' >> character for GSSAPI much as for SSL. At that time, we need to perform >> an *authentication* handshake (because GSSAPI will not do encryption >> before authenticating). I expect to use a consistent format for all >> GSSAPI packets - four bytes for length, and a payload. (I would prefer >> tagging them, but previously preference for not doing this has been >> expressed.) OK. >> Once GSSAPI authentication is complete, the normal handshake process can >> be tunneled through a GSSAPI encryption layer, as is done with TLS. The >> server will need to retain some of the earlier authentication data >> (e.g., to check that the presented user-name matches GSSAPI >> credentials), but there will be no authentication packets exchanged >> (more specifically, it will resemble the anonymous case). Authorization >> will be checked as normal, and we then proceed in the usual fashion, all >> over the GSSAPI tunnel. OK, that sounds good. >> On the server, I'll need to implement `hostgss` (by analogy to >> `hostssl`), and we'll want to lock authentication on those connections >> to GSSAPI-only. As well as hostnogss, but I guess that's part of the plan. >> Clients will explicitly probe for GSSAPI support as >> they do for TLS support (I look forward to the bikeshed on the order of >> these) and should have a parameter to require said support. One thing >> I'm not clear on is what our behavior should be when the user doesn't >> explicitly request GSSAPI and doesn't have a ccache - do we prompt? >> Skip probing? I'm not sure what the best option there is. I am not sure I get what you mean here. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Constraint merge and not valid status
Hello,
At Mon, 25 Jul 2016 18:21:27 +0900, Amit Langote
wrote in
<96fb8bca-57f5-e5a8-9630-79d4fc5b2...@lab.ntt.co.jp>
>
> Hello,
>
> On 2016/07/25 17:18, Kyotaro HORIGUCHI wrote:
> >
> > - Remove ccvalid condition from equalTupleDescs() to reduce
> >unnecessary cache invalidation or tuple rebuilding.
>
> The following commit introduced the ccvalid check:
>
> """
> commit c31305de5f5a4880b0ba2f5983025ef0210a3b2a
> Author: Noah Misch
> Date: Sun Mar 23 02:13:43 2014 -0400
>
> Address ccvalid/ccnoinherit in TupleDesc support functions.
>
> equalTupleDescs() neglected both of these ConstrCheck fields, and
> CreateTupleDescCopyConstr() neglected ccnoinherit. At this time, the
> only known behavior defect resulting from these omissions is constraint
> exclusion disregarding a CHECK constraint validated by an ALTER TABLE
> VALIDATE CONSTRAINT statement issued earlier in the same transaction.
> Back-patch to 9.2, where these fields were introduced.
> """
Wow.. Missed the obvious thing. Certainly relation cache must be
invalidated by a change of ccvalidated as the commit message.
> So, apparently RelationClearRelation() does intend to discard a cached
> TupleDesc if ccvalid changed in a transaction. Whereas,
> acquire_inherited_sample_rows() does not seem to depend on ccvalid being
> equal or not (or any member of TupleConstr for that matter). So maybe,
> instead of simply discarding the check (which does serve a purpose), we
> could make equalTupleDescs() parameterized on whether we want TupleConstr
> equality check to be performed or not. RelationClearRelation() can ask
> for the check to be performed by passing true for the parameter whereas
> acquire_inherited_sample_rows() and other callers can pass false. Perhaps
> something like the attached.
Hmm. It should resolve the problem. But the two comparisons seem
to be separate things. Constraints is not a part of tuple
description. relcache seems to be making misusage of the equality
of tuple descriptors.
So, how about splitting the original equalTupleDescs into
equalTupleDescs and equalTupleConstraints ?
regards,
--
Kyotaro Horiguchi
NTT Open Source Software Center
>From 5a1d570ac327702fea19ed18198efeb02632a2f3 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi
Date: Tue, 26 Jul 2016 11:02:48 +0900
Subject: [PATCH] Split equalTupleDescs into two functions.
equalTupleDescs intends to the two given tuple descriptors are
compatible on generating a tuple. Comaprison on constraints are
required to invalidate cache but it is a separate matter. So split it
into two new functions equalTupleDescs and equalTupleConstraints.
---
src/backend/access/common/tupdesc.c | 14 ++
src/include/access/tupdesc.h| 1 +
2 files changed, 15 insertions(+)
diff --git a/src/backend/access/common/tupdesc.c b/src/backend/access/common/tupdesc.c
index b56d0e3..1e42c87 100644
--- a/src/backend/access/common/tupdesc.c
+++ b/src/backend/access/common/tupdesc.c
@@ -411,6 +411,19 @@ equalTupleDescs(TupleDesc tupdesc1, TupleDesc tupdesc2)
/* attacl, attoptions and attfdwoptions are not even present... */
}
+ return true;
+}
+
+/*
+ * Compare constraints between two TupleDesc structures
+ */
+bool
+equalTupleConstraints(TupleDesc tupdesc1, TupleDesc tupdesc2)
+{
+ int i,
+j,
+n;
+
if (tupdesc1->constr != NULL)
{
TupleConstr *constr1 = tupdesc1->constr;
@@ -470,6 +483,7 @@ equalTupleDescs(TupleDesc tupdesc1, TupleDesc tupdesc2)
}
else if (tupdesc2->constr != NULL)
return false;
+
return true;
}
diff --git a/src/include/access/tupdesc.h b/src/include/access/tupdesc.h
index de18f74..58fa466 100644
--- a/src/include/access/tupdesc.h
+++ b/src/include/access/tupdesc.h
@@ -111,6 +111,7 @@ extern void DecrTupleDescRefCount(TupleDesc tupdesc);
} while (0)
extern bool equalTupleDescs(TupleDesc tupdesc1, TupleDesc tupdesc2);
+extern bool equalTupleConstraints(TupleDesc tupdesc1, TupleDesc tupdesc2);
extern void TupleDescInitEntry(TupleDesc desc,
AttrNumber attributeNumber,
--
1.8.3.1
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] AdvanceXLInsertBuffer vs. WAL segment compressibility
On Mon, Jul 25, 2016 at 11:21 PM, Chapman Flack wrote:
> The impression that leaves is of tools that relied too heavily
> on internal format knowledge to be viable outside of core, which
> have had at least periods of incompatibility with newer PG versions,
> and whose current status, if indeed any are current, isn't easy
> to find out.
WAL format has gone through a lot of changes in 9.4 as well. 9.3 has
as well introduced xlogreader.c which is what *any* client trying to
read WAL into an understandable format should use.
> And that, I assume, was also the motivation to put the zeroing
> in AdvanceXLInsertBuffer, eliminating the need for one narrow,
> specialized tool like pg{_clear,_compress,less}log{,tail}, so
> the job can be done with ubiquitous, bog standard (and therefore
> *very* exhaustively tested) tools like gzip.
Exactly, and honestly this has been a huge win to make such segments
more compressible.
> Even so, it still seems to me that a cheaper solution is a %e
> substitution in archive_command: just *tell* the command where
> the valid bytes end. Accomplishes the same thing as ~ 16 MB
> of otherwise-unnecessary I/O at the time of archiving each
> lightly-used segment.
>
> Then the actual zeroing could be suppressed to save I/O, maybe
> with a GUC variable, or maybe just when archive_command is seen
> to contain a %e. Commands that don't have a %e continue to work
> and compress effectively because of the zeroing.
This is over-complicating things for little gain. The new behavior of
filling in with zeros the tail of a segment makes things far better
when using gzip in archive_command.
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Curing plpgsql's memory leaks for statement-lifespan values
On Mon, Jul 25, 2016 at 6:04 PM, Tom Lane wrote: > I suppose that a fix based on putting PG_TRY blocks into all the affected > functions might be simple enough that we'd risk back-patching it, but > I don't really want to go that way. try/catch blocks aren't completely free, either, and PL/pgsql is not suffering from a deplorable excess of execution speed. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] PoC: Make it possible to disallow WHERE-less UPDATE and DELETE
On Fri, Jul 22, 2016 at 2:38 AM, David Fetter wrote: > I've renamed it to require_where and contrib-ified. I'm not sure that the Authors section is entirely complete. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] AdvanceXLInsertBuffer vs. WAL segment compressibility
On 07/25/16 22:09, Michael Paquier wrote: > This is over-complicating things for little gain. The new behavior of > filling in with zeros the tail of a segment makes things far better > when using gzip in archive_command. Then how about filling with actual zeros, instead of with mostly-zeros as is currently done? That would work just as well for gzip, and would not sacrifice the ability to do 100x better than gzip. -Chap -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] PoC: Make it possible to disallow WHERE-less UPDATE and DELETE
On Mon, Jul 25, 2016 at 11:12:24PM -0400, Robert Haas wrote: > On Fri, Jul 22, 2016 at 2:38 AM, David Fetter wrote: > > I've renamed it to require_where and contrib-ified. > > I'm not sure that the Authors section is entirely complete. Does this suit? Best, David. -- David Fetter http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Constraint merge and not valid status
Hello,
On 2016/07/26 11:05, Kyotaro HORIGUCHI wrote:
> At Mon, 25 Jul 2016 18:21:27 +0900, Amit Langote wrote:
>
>> So, apparently RelationClearRelation() does intend to discard a cached
>> TupleDesc if ccvalid changed in a transaction. Whereas,
>> acquire_inherited_sample_rows() does not seem to depend on ccvalid being
>> equal or not (or any member of TupleConstr for that matter). So maybe,
>> instead of simply discarding the check (which does serve a purpose), we
>> could make equalTupleDescs() parameterized on whether we want TupleConstr
>> equality check to be performed or not. RelationClearRelation() can ask
>> for the check to be performed by passing true for the parameter whereas
>> acquire_inherited_sample_rows() and other callers can pass false. Perhaps
>> something like the attached.
>
> Hmm. It should resolve the problem. But the two comparisons seem
> to be separate things. Constraints is not a part of tuple
> description. relcache seems to be making misusage of the equality
> of tuple descriptors.
>
> So, how about splitting the original equalTupleDescs into
> equalTupleDescs and equalTupleConstraints ?
Actually TupleConstr is *part* of the TupleDesc struct, which the
relcache.c tries to preserve in *whole* across a relcache flush, so it
seems perhaps cleaner to keep the decision centralized in one function:
keep_tupdesc = equalTupleDescs(relation->rd_att, newrel->rd_att);
...
/* preserve old tupledesc and rules if no logical change */
if (keep_tupdesc)
SWAPFIELD(TupleDesc, rd_att);
Also:
/*
* This struct is passed around within the backend to describe the
* structure of tuples. For tuples coming from on-disk relations, the
* information is collected from the pg_attribute, pg_attrdef, and
* pg_constraint catalogs.
...
typedef struct tupleDesc
{
It would perhaps have been clear if there was a rd_constr field that
relcache.c independently managed.
OTOH, I tend to agree that other places don't quite need the whole thing
(given also that most other callers except acquire_inherit_sample_rows
compare transient row types)
Thanks,
Amit
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_basebackup wish list
On Wed, Jul 13, 2016 at 3:06 AM, Jeff Janes wrote:
> On Tue, Jul 12, 2016 at 10:48 AM, Peter Eisentraut
> wrote:
>> On 7/12/16 12:53 PM, Jeff Janes wrote:
>>> The --help message for pg_basebackup says:
>>>
>>> -Z, --compress=0-9 compress tar output with given compression level
>>>
>>> But -Z0 is then rejected as 'invalid compression level "0"'. The real
>>> docs do say 1-9, only the --help message has this bug. Trivial patch
>>> attached.
>>
>> pg_dump --help and man page say it supports 0..9. Maybe we should make
>> that more consistent.
>
> pg_dump actually does support -Z0, though. Well, sort of. It outputs
> plain text. Rather than plain text wrapped in some kind of dummy gzip
> header, which is what I had naively expected.
>
> Is that what -Z0 in pg_basebackup should do as well, just output
> uncompressed tar data, and not add the ".gz" to the "base.tar" file
> name?
Yes, I think. What about the attached patch?
Regards,
--
Fujii Masao
*** a/doc/src/sgml/ref/pg_basebackup.sgml
--- b/doc/src/sgml/ref/pg_basebackup.sgml
***
*** 364,370 PostgreSQL documentation
Enables gzip compression of tar file output, and specifies the
! compression level (1 through 9, 9 being best
compression). Compression is only available when using the tar
format.
--- 364,370
Enables gzip compression of tar file output, and specifies the
! compression level (0 through 9, 0 being no compression and 9 being best
compression). Compression is only available when using the tar
format.
*** a/src/bin/pg_basebackup/pg_basebackup.c
--- b/src/bin/pg_basebackup/pg_basebackup.c
***
*** 2073,2079 main(int argc, char **argv)
break;
case 'Z':
compresslevel = atoi(optarg);
! if (compresslevel <= 0 || compresslevel > 9)
{
fprintf(stderr, _("%s: invalid compression level \"%s\"\n"),
progname, optarg);
--- 2073,2079
break;
case 'Z':
compresslevel = atoi(optarg);
! if (compresslevel < 0 || compresslevel > 9)
{
fprintf(stderr, _("%s: invalid compression level \"%s\"\n"),
progname, optarg);
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
