Re: [HACKERS] Parallel Index Scans

[Amit Kapila]

On Sat, Jan 21, 2017 at 1:15 AM, Robert Haas  wrote:
> On Fri, Jan 20, 2017 at 9:29 AM, Amit Kapila  wrote:
>> Sure, if scan keys have changed then we can't continue, but this is
>> the case where runtime keys are first time initialized.
>>
>> if (node->iss_NumRuntimeKeys != 0 && !node->iss_RuntimeKeysReady)
>>
>> In the above check, the second part of the check
>> (!node->iss_RuntimeKeysReady) ensures that it is for the first time.
>> Now, let me give you an example to explain what bad can happen if we
>> allow resetting parallel scan in this case.  Consider a query like
>> select * from t1 where c1 < parallel_index(10);, in this if we allow
>> resetting parallel scan descriptor during first time initialization of
>> runtime keys, it can easily corrupt the parallel scan state.  Suppose
>> leader has taken the lead and is scanning some page and worker reaches
>> to initialize its keys in ExecReScanIndexScan(), if worker resets the
>> parallel scan, then it will corrupt the state of the parallel scan
>> state.
>
> Hmm, I see.  So the problem if I understand it correctly is that every
> participating process needs to update the backend-private state for
> the runtime keys and only one of those processes can update the shared
> state.  But in the case of a "real" rescan, even the shared state
> needs to be reset.  OK, makes sense.
>

Exactly.

> Why does btparallelrescan cater to the case where scan->parallel_scan
> == NULL?  I would assume it should never get called in that case.
>

Okay, will modify the patch accordingly.

> Also, I think ExecReScanIndexScan needs significantly better comments.
> After some thought I see what's it's doing - mostly anyway - but I was
> quite confused at first.  I still don't completely understand why it
> needs this if-test:
>
> +   /* reset (parallel) index scan */
> +   if (node->iss_ScanDesc)
> +   {
>

I have mentioned the reason towards the end of the e-mail [1] (Refer
line, This is required because ..).  Basically, this is required to
make plans like below work sanely.

Nested Loop
  -> Seq Scan on a
  -> Gather
-> Parallel Index Scan on b
  Index Cond: b.x = 15

I understand that such plans don't make much sense, but we do support
them and I have seen somewhat similar plan getting select in TPC-H
benchmark Let me know if this needs more explanation.

>
> I think it's a good idea to put all three of those functions together
> in the listing, similar to what we did in
> 69d34408e5e7adcef8ef2f4e9c4f2919637e9a06 for FDWs.  After all they are
> closely related in purpose, and it may be easiest to understand if
> they are next to each other in the listing.  I suggest that we move
> them to the end in IndexAmRoutine similar to the way FdwRoutine was
> done; in other words, my idea is to make the structure consistent with
> the way that I revised the documentation instead of making the
> documentation consistent with the order you picked for the structure
> members.  What I like about that is that it gives a good opportunity
> to include some general remarks on parallel index scans in a central
> place, as I did in that patch.  Also, it makes it easier for people
> who care about parallel index scans to find all of the related things
> (since they are together) and for people who don't care about them to
> ignore it all (for the same reason).  What do you think about that
> approach?
>

Sounds sensible.  Updated patch based on that approach is attached.  I
will rebase the remaining work based on this patch and send them
separately.

[1] - 
https://www.postgresql.org/message-id/CAA4eK1%2BnBiCxtxcNuzpaiN%2BnrRrRB5YDgoaqb3hyn%3DYUxL-%2BOw%40mail.gmail.com
-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com


parallel-generic-index-scan.1.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

Re: [HACKERS] Failure in commit_ts tap tests

[Pavan Deolasee]

On Sat, Jan 21, 2017 at 12:45 AM, Tom Lane  wrote:
>
>
> Even more interesting, the warning appears as-expected in stripped down
> test cases, eg
>
> $ perl -e 'use warnings; use Test::More; ok("Foo" ne "bar", "ok");'
> ok 1 - ok
> # Tests were run but no plan was declared and done_testing() was not seen.
>
> $ perl -e 'use warnings; use Test::More; ok("Foo" != "bar", "ok");'
> Argument "bar" isn't numeric in numeric ne (!=) at -e line 1.
> Argument "Foo" isn't numeric in numeric ne (!=) at -e line 1.
> not ok 1 - ok
> #   Failed test 'ok'
> #   at -e line 1.
> # Tests were run but no plan was declared and done_testing() was not seen.
>
> I really don't understand why this complains but the same perl version
> is happy with (the previous coding in) 004_restart.pl.  Perl bug?
>
>
I think I understand why it's only affecting me and not others.
I've PGDATESTYLE set to "Postgres, MDY" in my bashrc and that formats the
commit timestamp as "Fri Jan 20 07:59:52.322811 2017 PST". If I unset that,
the result comes in a format such as  "2017-01-20 21:31:47.766371-08".
Looks like perl doesn't throw an error if it can parse the leading part of
the string as a numeric. It still throws a warning, but the test passes.

Thanks,
Pavan

-- 
 Pavan Deolasee   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Re: [HACKERS] Packages: Again

[Pavel Stehule]

2017-01-20 17:01 GMT+01:00 Joshua D. Drake :

> On 01/17/2017 09:26 AM, Robert Haas wrote:
>
>> On Fri, Jan 13, 2017 at 7:24 PM, Peter Geoghegan  wrote:
>>
>>> MERGE isn't UPSERT, and isn't even in competition with UPSERT as a
>>> feature. I've written reams of text explaining why this is so in
>>> precise detail, ...
>>>
>>
>
> Hello,
>
> This is the webinar that started this whole thread (well the original
> thread, not this weird MERGE/UPSERT stuff):
>
> https://www.commandprompt.com/blog/postgresql_for_oracle_people/
>
> Thank you to everyone that responded. You will see in this Webinar that at
> least from the Oracle people perspective, PostgreSQL is not an option
> unless it has packages.
>
> The other item that people bring up a few times is Oracle Forms but as
> that is actually external (although dependent) on Oracle, I don't see that
> as our responsibility.


I see there request on package functions, that can be realised with our
schemas - schema function is +/- equal - and better support for package
variables - there is a patch for schema session variables.

Regards

Pavel


>
>
> Sincerely,
>
> JD
>
>
> --
> 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.
>

Re: [HACKERS] Off-by-one oddity in minval for decreasing sequences

[Michael Paquier]

On Sat, Jan 21, 2017 at 10:20 AM, Peter Eisentraut
 wrote:
> On 1/6/17 2:15 PM, Daniel Verite wrote:
>> I notice that there's a preexisting
>> oddity in the fact that sequences created with a negative increment
>> in current releases initialize the minval to -(2^63)+1 instead of -2^63,
>> the actual lowest value for a bigint.
>
> I think that had to do with that we had to play games to work around the
> lack of proper int64 support, and various weird code has developed over
> time because of that.  I think we should fix it if we can.
>
> The attached patch fixes the default minimum value to use the proper
> int64 min value.
>
> With this patch, when upgrading with pg_dump, descending sequences with
> the previous default minimum value would be kept with that
> now-not-default value.  We could alternative adjust those sequences to
> the new default value.

This patch looks acceptable 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] Measuring replay lag

[Thomas Munro]

On Tue, Jan 17, 2017 at 7:45 PM, Fujii Masao  wrote:
> On Thu, Dec 22, 2016 at 6:14 AM, Thomas Munro
>  wrote:
>> On Thu, Dec 22, 2016 at 2:14 AM, Fujii Masao  wrote:
>>> I agree that the capability to measure the remote_apply lag is very useful.
>>> Also I want to measure the remote_write and remote_flush lags, for example,
>>> in order to diagnose the cause of replication lag.
>>
>> Good idea.  I will think about how to make that work.  There was a
>> proposal to make writing and flushing independent[1].  I'd like that
>> to go in.  Then the write_lag and flush_lag could diverge
>> significantly, and it would be nice to be able to see that effect as
>> time (though you could already see it with LSN positions).
>>
>>> For that, what about maintaining the pairs of send-timestamp and LSN in
>>> *sender side* instead of receiver side? That is, walsender adds the pairs
>>> of send-timestamp and LSN into the buffer every sampling period.
>>> Whenever walsender receives the write, flush and apply locations from
>>> walreceiver, it calculates the write, flush and apply lags by comparing
>>> the received and stored LSN and comparing the current timestamp and
>>> stored send-timestamp.
>>
>> I thought about that too, but I couldn't figure out how to make the
>> sampling work.  If the primary is choosing (LSN, time) pairs to store
>> in a buffer, and the standby is sending replies at times of its
>> choosing (when wal_receiver_status_interval has been exceeded), then
>> you can't accurately measure anything.
>
> Yeah, even though the primary stores (100, 2017-01-17 00:00:00) as the pair of
> (LSN, timestamp), for example, the standby may not send back the reply for
> LSN 100 itself. The primary may receive the reply for larger LSN like 200,
> instead. So the measurement of the lag in the primary side would not be so
> accurate.
>
> But we can calculate the "sync rep" lag by comparing the stored timestamp of
> LSN 100 and the timestamp when the reply for LSN 200 is received. In sync rep,
> since the transaction waiting for LSN 100 to be replicated is actually 
> released
> after the reply for LSN 200 is received, the above calculated lag is basically
> accurate as sync rep lag.
>
> Therefore I'm still thinking that it's better to maintain the pairs of LSN
> and timestamp in the *primary* side. Thought?

Ok.  I see that there is a new compelling reason to move the ring
buffer to the sender side: then I think lag tracking will work
automatically for the new logical replication that just landed on
master.  I will try it that way.  Thanks for the feedback!

-- 
Thomas Munro
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] Off-by-one oddity in minval for decreasing sequences

[Peter Eisentraut]

On 1/6/17 2:15 PM, Daniel Verite wrote:
> I notice that there's a preexisting
> oddity in the fact that sequences created with a negative increment
> in current releases initialize the minval to -(2^63)+1 instead of -2^63,
> the actual lowest value for a bigint.

I think that had to do with that we had to play games to work around the
lack of proper int64 support, and various weird code has developed over
time because of that.  I think we should fix it if we can.

The attached patch fixes the default minimum value to use the proper
int64 min value.

With this patch, when upgrading with pg_dump, descending sequences with
the previous default minimum value would be kept with that
now-not-default value.  We could alternative adjust those sequences to
the new default value.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>From 5c0223292c1ad065d73294c99fae3da60407f001 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Fri, 20 Jan 2017 19:58:56 -0500
Subject: [PATCH] Fix default minimum value for descending sequences

For some reason that is lost in history, a descending sequence would
default its minimum value to -2^63+1 (-PG_INT64_MAX) instead of
-2^63 (PG_INT64_MIN), even though explicitly specifying a minimum value
of -2^63 would work.  Fix this inconsistency by using the full range by
default.

found by Daniel Verite 
---
 doc/src/sgml/ref/create_sequence.sgml | 2 +-
 src/backend/commands/sequence.c   | 4 ++--
 src/bin/pg_dump/pg_dump.c | 4 ++--
 src/include/pg_config_manual.h| 6 --
 4 files changed, 5 insertions(+), 11 deletions(-)

diff --git a/doc/src/sgml/ref/create_sequence.sgml b/doc/src/sgml/ref/create_sequence.sgml
index 62ae379226..86ff018c4b 100644
--- a/doc/src/sgml/ref/create_sequence.sgml
+++ b/doc/src/sgml/ref/create_sequence.sgml
@@ -133,7 +133,7 @@ Parameters
   the minimum value a sequence can generate. If this clause is not
   supplied or NO MINVALUE is specified, then
   defaults will be used.  The defaults are 1 and
-  -263-1 for ascending and descending sequences,
+  -263 for ascending and descending sequences,
   respectively.
  
 
diff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequence.c
index 2de46c270e..bafa95ce8f 100644
--- a/src/backend/commands/sequence.c
+++ b/src/backend/commands/sequence.c
@@ -1352,7 +1352,7 @@ init_params(ParseState *pstate, List *options, bool isInit,
 	else if (isInit || max_value != NULL)
 	{
 		if (seqform->seqincrement > 0)
-			seqform->seqmax = SEQ_MAXVALUE;		/* ascending seq */
+			seqform->seqmax = PG_INT64_MAX;		/* ascending seq */
 		else
 			seqform->seqmax = -1;	/* descending seq */
 		seqdataform->log_cnt = 0;
@@ -1369,7 +1369,7 @@ init_params(ParseState *pstate, List *options, bool isInit,
 		if (seqform->seqincrement > 0)
 			seqform->seqmin = 1; /* ascending seq */
 		else
-			seqform->seqmin = SEQ_MINVALUE;		/* descending seq */
+			seqform->seqmin = PG_INT64_MIN;		/* descending seq */
 		seqdataform->log_cnt = 0;
 	}
 
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 0bb363957a..3a698afac5 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -15894,8 +15894,8 @@ dumpSequence(Archive *fout, TableInfo *tbinfo)
 	/* Make sure we are in proper schema */
 	selectSourceSchema(fout, tbinfo->dobj.namespace->dobj.name);
 
-	snprintf(bufm, sizeof(bufm), INT64_FORMAT, SEQ_MINVALUE);
-	snprintf(bufx, sizeof(bufx), INT64_FORMAT, SEQ_MAXVALUE);
+	snprintf(bufm, sizeof(bufm), INT64_FORMAT, PG_INT64_MIN);
+	snprintf(bufx, sizeof(bufx), INT64_FORMAT, PG_INT64_MAX);
 
 	if (fout->remoteVersion >= 10)
 	{
diff --git a/src/include/pg_config_manual.h b/src/include/pg_config_manual.h
index c07907145a..f3b35297d1 100644
--- a/src/include/pg_config_manual.h
+++ b/src/include/pg_config_manual.h
@@ -51,12 +51,6 @@
 #define PARTITION_MAX_KEYS	32
 
 /*
- * Set the upper and lower bounds of sequence values.
- */
-#define SEQ_MAXVALUE	PG_INT64_MAX
-#define SEQ_MINVALUE	(-SEQ_MAXVALUE)
-
-/*
  * When we don't have native spinlocks, we use semaphores to simulate them.
  * Decreasing this value reduces consumption of OS resources; increasing it
  * may improve performance, but supplying a real spinlock implementation is
-- 
2.11.0


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

[HACKERS] remote_apply for logical replication?

[Thomas Munro]

Hi,

In src/backend/replication/logical/worker.c:

pq_sendbyte(reply_message, 'r');
pq_sendint64(reply_message, recvpos);   /* write */
pq_sendint64(reply_message, flushpos);  /* flush */
pq_sendint64(reply_message, writepos);  /* apply */

Is 'writepos' really applied?  Why do we report the merely received
position as written?

I haven't tried any of this stuff out yet so this may be a stupid
question, but I'm wondering: should xinfo &
XACT_COMPLETION_APPLY_FEEDBACK trigger logical replication to send a
reply, so that synchronous_commit = remote_apply would work?

-- 
Thomas Munro
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] Valgrind-detected bug in partitioning code

[Tom Lane]

Robert Haas  writes:
> The difference is that those other equalBLAH functions call a
> carefully limited amount of code whereas, in looking over the
> backtrace you sent, I realized that equalPartitionDescs is calling
> partition_bounds_equal which does this:
>cmpval =
> DatumGetInt32(FunctionCall2Coll(>partsupfunc[j],
>   key->partcollation[j],
>   b1->datums[i][j],
>   b2->datums[i][j]))

Ah, gotcha.

> That's of course opening up a much bigger can of worms.  But apart
> from the fact that it's unsafe, I think it's also wrong, as I said
> upthread.  I think calling datumIsEqual() there should be better all
> around.  Do you think that's unsafe here?

That sounds like a plausible solution.  It is safe in the sense of
being a bounded amount of code.  It would return "false" in various
interesting cases like toast pointer versus detoasted equivalent,
but I think that would be fine in this application.

It would probably be a good idea to add something to datumIsEqual's
comment to the effect that trying to make it smarter would be a bad idea,
because some callers rely on it being stupid.

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] increasing the default WAL segment size

[Michael Paquier]

On Sat, Jan 21, 2017 at 4:50 AM, Robert Haas  wrote:
> On Fri, Jan 20, 2017 at 2:34 AM, Michael Paquier
>  wrote:
>> On Fri, Jan 20, 2017 at 12:49 AM, Robert Haas  wrote:
>>> On Wed, Jan 18, 2017 at 12:42 PM, Robert Haas  wrote:
 Problems 2-4 actually have to do with a DestReceiver of type
 DestRemote really, really wanting to have an associated Portal and
 database connection, so one approach is to create a stripped-down
 DestReceiver that doesn't care about those things and then passing
 that to GetPGVariable.
>>>
>>> I tried that and it worked out pretty well, so I'm inclined to go with
>>> this approach.  Proposed patches attached.  0001 adds the new
>>> DestReceiver type, and 0002 is a revised patch to implement the SHOW
>>> command itself.
>>>
>>> Thoughts, comments?
>>
>> This looks like a sensible approach to me. DestRemoteSimple could be
>> useful for background workers that are not connected to a database as
>> well. Isn't there a problem with PGC_REAL parameters?
>
> No, because the output of SHOW is always of type text, regardless of
> the type of the GUC.

Thinking about that over night, that looks pretty nice. What would be
nicer though would be to add INT8OID and BYTEAOID in the list, and
convert as well the other replication commands. At the end, I think
that we should finish by being able to remove all pq_* routine
dependencies in walsender.c, saving quite a couple of lines.
-- 
Michael


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

Updating MATERIALIZED VIEWs (Re: [HACKERS] delta relations in AFTER triggers)

[Nico Williams]

[Looking at your patch I see that it's not quite related to MVs, so I'm
changing the Subject.  Apologies for the noise.]

[Responding out of order.]

On Fri, Jan 20, 2017 at 03:37:20PM -0600, Kevin Grittner wrote:
> On Fri, Jan 20, 2017 at 2:08 PM, Nico Williams  wrote:
> > BTW, automatic updates of certain types of MVs should be easy: add
> > constraints based on NEW/OLD rows from synthetic triggers to the
> > underlying query.
> 
> Convincing me that this is a good idea for actual MVs, versus
> pseudo-MVs using tables, would be an uphill battle.  [...]

I don't think it's necessary, and I would not insist on it.

My alternative MV implementation lets _me_ choose when to update an MV
synchronously, and when to defer refreshes, by using [hand-coded]
triggers.  This is good enough for me.

If these triggers could be automatically generated, that sure would be
nice, but some control would be needed over when to update the MV vs.
mark it as needing a refresh.

> > Our intention is to contribute this.  We're willing to sign
> > reasonable contribution agreements.
> 
> Posting a patch to these lists constitutes an assertion that you
> have authority to share the IP, and are doing so.  Referencing a
> URL is a bit iffy, since it doesn't leave an archival copy of the
> contribution under the community's control.

Fair enough.  I'll post the source file itself.  I've not done the work
of properly integrating it because I need to gauge interest first,
before dedicating a lot of effort to it.

> I am dubious, though, of the approach in general, as stated above.

I'm using this _now_.  With a caveat:

a) the trigger functions needed to either mark an MV as needing a
refresh, or else to update it directly, are hand-coded, and

b) I chose which operations yield synchronous MV updates and which defer
to a refresh.

The MV, in my scheme, is really just a table with triggers that update a
deltas table the same way that a refresh would.  A refresh locks the
table, disables those triggers, populates another table with the current
output of the underlying view, compares to the previous materialization,
and lastly generates, records, and applies deltas to the
materialization.

To give an example, adding a user to a group -> generally fast; deleting
a user (and thus all their group memberships) -> potentially very slow.

The "add a user to a group" case can then yield near real-time updates
of external caches, while the other case results in a deferred REFRESH
so as to not slow down the current transaction.  The deferred REFRESH is
not deferred too long, so the net effect is still very fast updates of
external caches.

> > However, there is a bug in the query planner that prevents this
> > from being very fast.  At some point I want to tackle that bug.
> 
> What bug is that?

I... asked for help on the IRC #postgresql channel.  I never posted here
about it.

Revisiting it now... the main problem was query _preparation time_, not
execution time.  So perhaps not so bad.  Still, it's worth looking into.

The query was something like this:

SELECT v.data->'verb_name' || '^' || (r.data->'named_acl_name') AS 
grant_name,
   grantee.right_entity_id AS grantee_id
FROM relationships grantee
JOIN relationships grant ON
 grantee.left_entity_id = grant.right_entity_id AND
 grantee.relationship_type_id IN (10421, 10431, 13591, 13921)
 AND grant.relationship_type_id = 10331
JOIN relationships perm_actions ON
 grantee.left_entity_id = perm_actions.right_entity_id AND
 perm_actions.relationship_type_id = 10381
JOIN relationships verb_in_vs ON
 verb_in_vs.right_entity_id = perm_actions.left_entity_id AND
 verb_in_vs.relationship_type_id = 10371
JOIN entities v ON v.id = verb_in_vs.left_entity_id
JOIN entities r ON r.id = grant.left_entity_id;

  (This query uses a bit of an EAV schema.  There's an "entities" table
  with an hstore column for storing attributes ("data") and another
  table, "relationships" that has (relationship_type_id, left_entity_id,
  right_entity_id) columns and which is indexed by both, left_entity_id
  and right_entity_id.  EAV schemas hide relevant information from the
  query planner, so there is that.)

The query plan for this is about as fast as one could hope.  After all,
it has to scan many of the rows.

Now suppose we were adding a new 'grantee' and wanted to generate the
additions that would result in the MV.  We could add this constraint to
the query:

WHERE grantee.left_entity_id = NEW.left_entity_id AND
grantee.right_entity_id = NEW.right_entity_id;

Now we've basically [almost] fully-specified the primary key for the
grantee table source.

The resulting query plan is actually pretty good.  It has the grantee
table source as the first table source in the inner-most loop.

If I re-write the query using WITH CTEs to get 

Re: [HACKERS] [PATCH] Add GUCs for predicate lock promotion thresholds

[Kevin Grittner]

A couple things occurred to me after hitting "Send".

In addition to the prior 2 points:

(3)  The documentation for max_pred_locks_per_relation needs a fix.
Both page locks and tuple locks for the relation are counted toward
the limit.

In releases prior to this patch, max_pred_locks_per_relation was
calculated as "max_pred_locks_per_transaction / 2".  I know that
people have sometimes increased max_pred_locks_per_transaction
specifically to raise the limit on locks within a relation before
the promotion to relation granularity occurs.  It seems kinda
anti-social not to support a special value for continuing that
behavior or, if we don't want to do that, at least modifying
pg_upgrade to set max_pred_locks_per_relation to the value that was
in effect in the old version.  In any event, it seems more like
autovacuum_work_mem or autovacuum_vacuum_cost_limit than like
effective_cache_size.

Kevin Grittner
EDB: 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] [PATCH] Add GUCs for predicate lock promotion thresholds

[Kevin Grittner]

On Thu, Jan 5, 2017 at 12:19 PM, Tom Lane  wrote:
> ilm...@ilmari.org (Dagfinn Ilmari =?utf-8?Q?Manns=C3=A5ker?=) writes:
>> ilm...@ilmari.org (Dagfinn Ilmari Mannsåker) writes:
>>> One thing I don't like about this patch is that if a user has increased
>>> max_pred_locks_per_transaction, they need to set
>>> max_pred_locks_per_relation to half of that to retain the current
>>> behaviour, or they'll suddenly find themselves with a lot more relation
>>> locks.  If it's possible to make a GUCs default value dependent on the
>>> value of another, that could be a solution.  Otherwise, the page lock
>>> threshold GUC could be changed to be expressed as a fraction of
>>> max_pred_locks_per_transaction, to keep the current behaviour.
>
>> Another option would be to have a special sentinel (e.g. -1) which is
>> the default, and keeps the current behaviour.
>
> FWIW, interdependent GUC defaults are generally unworkable.
> See commit a16d421ca and the sorry history that led up to it.
> I think you could make it work as a fraction, though.

I think that, ultimately, both an fraction of actual (the number of
tuples on a page or the number of pages in a relation) *and* an
absolute number (as this patch implements) could be useful.  The
former would be more "adaptable" -- providing reasonable behavior
for different sized tuples and different sized tables, while the
latter would prevent a single table with very small tuples or a lot
of pages from starving all other uses.  This patch implements the
easier part, and is likely to be very useful to many users without
the other part, so I think it is worth accepting as a step in the
right direction, and consistent with not letting the good be the
enemy of the perfect.

There are a couple minor formatting issues I can clean up as
committer, but there are a couple more substantive things to note.

(1)  The new GUCs are named max_pred_locks_per_*, but the
implementation passes them unmodified from a function specifying at
what count the lock should be promoted.  We either need to change
the names or (probably better, only because I can't think of a good
name for the other way) return the GUC value + 1 from the function.
Or maybe change the function name and how the returned number is
used, if we care about eliminating the overhead of the increment
instruction.  That actually seems least confusing.

(2)  The new GUCs are declared and documented to be set only on
startup.  This was probably just copied from
max_pred_locks_per_transaction, but that sets an allocation size
while these don't.  I think these new GUCs should be PGC_SIGHUP,
and documented to change on reload.

Other than that, I think it is in good shape and I am would feel
good about committing it.  Of course, if there are any objections
to it, I will not go ahead without reaching consensus.  I am on
vacation next week, so I would not do so before then; in fact I may
not have a chance to respond to any comments before then.  If the
author wishes to make these changes, great; otherwise I can do it
before commit.

I will mark this Ready for Committer.

--
Kevin Grittner
EDB: 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] Valgrind-detected bug in partitioning code

[Robert Haas]

On Fri, Jan 20, 2017 at 4:30 PM, Tom Lane  wrote:
> Stephen Frost  writes:
>> * Tom Lane (t...@sss.pgh.pa.us) wrote:
>>> Robert Haas  writes:
 Hmm.  That's bad.  I kind of wonder how sane it is to think that we
 can invoke SQL-callable functions during a relcache reload,
>
>>> You're doing WHAT?
>
>> Uh.  +1.
>
> Now that I've calmed down a bit: the right way to do this sort of thing is
> simply to flush the invalidated data during reload, and recompute it when
> it is next requested, which necessarily will be inside a valid
> transaction.  Compare e.g. the handling of the lists of a relation's
> indexes.

The existing handling of partition descriptors is modeled on and very
similar to the existing handling for other types of objects:


keep_tupdesc = equalTupleDescs(relation->rd_att,
newrel->rd_att);
keep_rules = equalRuleLocks(relation->rd_rules,
newrel->rd_rules);
keep_policies = equalRSDesc(relation->rd_rsdesc,
newrel->rd_rsdesc);
keep_partkey = (relation->rd_partkey != NULL);
keep_partdesc = equalPartitionDescs(relation->rd_partkey,

 relation->rd_partdesc,

 newrel->rd_partdesc);

And I think the reason is the same too, namely, if we've got a pointer
into partition descriptor in the relcache, we don't want that to
suddenly get swapped out and replaced with a pointer to an equivalent
data structure at a different address, because then our pointer will
be dangling.  That seems fine as far as it goes.

The difference is that those other equalBLAH functions call a
carefully limited amount of code whereas, in looking over the
backtrace you sent, I realized that equalPartitionDescs is calling
partition_bounds_equal which does this:

   cmpval =
DatumGetInt32(FunctionCall2Coll(>partsupfunc[j],

  key->partcollation[j],

  b1->datums[i][j],

  b2->datums[i][j]))


That's of course opening up a much bigger can of worms.  But apart
from the fact that it's unsafe, I think it's also wrong, as I said
upthread.  I think calling datumIsEqual() there should be better all
around.  Do you think that's unsafe here?

-- 
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] delta relations in AFTER triggers

[Kevin Grittner]

On Fri, Jan 20, 2017 at 2:08 PM, Nico Williams  wrote:
> On Fri, Jan 20, 2017 at 01:37:33PM -0600, Kevin Grittner wrote:

>> There is currently plenty of room for pseudo-MV implementations,
>> and may be for a while.  It's a good indication of the need for the
>> feature in core.  An implementation in the guts of core can have
>> advantages that nothing else can, of course.  For example, for
>> eager application of the deltas, nothing will be able to beat
>> capturing tuples already in RAM and being looked at for possible
>> trigger firing into a RAM-with-spill-to-disk tuplestore.
>
> BTW, automatic updates of certain types of MVs should be easy: add
> constraints based on NEW/OLD rows from synthetic triggers to the
> underlying query.

Convincing me that this is a good idea for actual MVs, versus
pseudo-MVs using tables, would be an uphill battle.  I recognize
the need to distinguish between MVs which contain recursive CTEs in
their definitions and MVs that don't, so that the DRed algorithm
can be used for the former and the counting algorithm for the
latter; but firing triggers for row-at-a-time maintenance is not
going to be efficient for very many cases, and the cost of
identifying those cases to handle them differently is probably
going to exceed any gains.  Comparative benchmarks, once there is
an implementation using set-based techniques, could potentially
convince me; but there's not much point arguing about it before
that exists.  :-)

> However, there is a bug in the query planner that prevents this
> from being very fast.  At some point I want to tackle that bug.

What bug is that?

> Basically, the planner does not notice that a table source in a
> join has a lookup key sufficiently well-specified by those additional
> constraints that it should be the first table source in the outermost
> loop.

Is that a description of what you see as the bug?  Can you give an
example, to clarify the point?

I am dubious, though, of the approach in general, as stated above.

>> I don't have time to review what you've done right now, but will
>> save that link to look at later, if you give permission to borrow
>> from it (with proper attribution, of course) if there is something
>> that can advance what I'm doing.  If such permission is not
>> forthcoming, I will probably avoid looking at it, to avoid any
>> possible copyright issues.
>
> Our intention is to contribute this.  We're willing to sign
> reasonable contribution agreements.

Posting a patch to these lists constitutes an assertion that you
have authority to share the IP, and are doing so.  Referencing a
URL is a bit iffy, since it doesn't leave an archival copy of the
contribution under the community's control.

> I'd appreciate a review, for sure.  Thanks!

Would it be possible to get your approach running using tables
and/or (non-materialized) views as an extension?  A trigger-based
way to maintain pseudo-MVs via triggers might make an interesting
extension, possibly even included in contrib if it could be shown
to have advantages over built-in MVs for some non-trivial
applications.

> There's a gotcha w.r.t. NULL columns, but it affects the built-in
> REFRESH as well, IIRC.  The commentary in our implementation
> discusses that in more detail.

Could you report that on a new thread on the lists?  I've seen
comments about such a "gotcha", but am not clear on the details.
It probably deserves its own thread.  Once understood, we can
probably fix it.

Thanks!

--
Kevin Grittner
EDB: 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] Vacuum: allow usage of more than 1GB of work mem

[Alvaro Herrera]

Alvaro Herrera wrote:

> There was no discussion whatsoever of the "prefetch" patch in this
> thread; and as far as I can see, nobody even mentioned such an idea in
> the thread.  This prefetch patch appeared out of the blue and there was
> no discussion about it that I can see.  Now I was about to push it after
> some minor tweaks, and went to search where was its justification, only
> to see that there was none.  Did anybody run tests with this patch?
> 
> I attach it now one more time.  My version is based on the latest
> Claudio posted at
> https://postgr.es/m/CAGTBQpa464RugxYwxLTtDi=syv9gngfcjk8uzb2fr6nddqu...@mail.gmail.com
> I don't know if there are differences to the version first posted.
> I only changed the magic number 32 to a #define, and added a
> CHECK_FOR_INTERRUPTS in the prefetching loop.

Ah, I found the justification here:
https://www.postgresql.org/message-id/flat/CAGTBQpa464RugxYwxLTtDi%3DSyv9GnGFcJK8uZb2fR6NDDqULaw%40mail.gmail.com#CAGTBQpbayY-t5-ySW19yQs1dBqvV6dm8dmdpTv_FWXmDC0A0cQ%40mail.gmail.com
apparently the truncate scan is 4x-6x faster with this prefetching.
Nice!

-- 
Álvaro Herrerahttps://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] Logical Replication WIP

[Petr Jelinek]

On 20/01/17 17:23, Petr Jelinek wrote:
> On 20/01/17 15:08, Peter Eisentraut wrote:
>> On 1/19/17 5:01 PM, Petr Jelinek wrote:
>>> There were some conflicting changes committed today so I rebased the
>>> patch on top of them.
>>>
>>> Other than that nothing much has changed, I removed the separate sync
>>> commit patch, included the rename patch in the patchset and fixed the
>>> bug around pg_subscription catalog reported by Erik Rijkers.
>>
>> Committed.  I haven't reviewed the rename patch yet, so I'll get back to
>> that later.
>>
> 
> Hi,
> 
> Thanks!
> 
> Here is fix for the dependency mess.
> 

Álvaro pointed out off list couple of issues with how we handle
interruption of commands that connect to walsender.

a) The libpqwalreceiver.c does blocking connect so it's impossible to
cancel CREATE SUBSCRIPTION which is stuck on connect. This is btw
preexisting problem and applies to walreceiver as well. I rewrote the
connect function to use asynchronous API (patch 0001).

b) We can cancel in middle of the command (when stuck in
libpqrcv_PQexec) but the connection to walsender stays open which in
case we are waiting for snapshot can mean that it will stay idle in
transaction. I added PG_TRY wrapper which disconnects on error around
this (patch 0002).

And finally, while testing these two I found bug in walsender StringInfo
initialization (or lack there of). There are 3 static StringInfo buffers
that are initialized in WalSndLoop. Problem with that is that they can
be in some rare scenarios used from CreateReplicationSlot (and IMHO
StartLogicalReplication) before WalSndLoop is called which causes
segfault of walsender. This is rare because it only happens when
downstream closes connection during logical decoding initialization.

Since it's not exactly straight forward to find when these need to be
initialized based on commands, I decided to move the initialization code
to exec_replication_command() since that's always called before anything
so that makes it much less error prone (patch 0003).

The 0003 should be backpatched all the way to 9.4 where multiple
commands started using those buffers.

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services
From 0c18831557c963bdd4d03ef5d8f3c4ace1a51d0e Mon Sep 17 00:00:00 2001
From: Petr Jelinek 
Date: Fri, 20 Jan 2017 21:20:56 +0100
Subject: [PATCH 1/3] Use asynchronous connect API in libpqwalreceiver

---
 src/backend/postmaster/pgstat.c|  4 +-
 .../libpqwalreceiver/libpqwalreceiver.c| 58 +-
 src/include/pgstat.h   |  2 +-
 3 files changed, 59 insertions(+), 5 deletions(-)

diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
index 7176cf1..19ad6b5 100644
--- a/src/backend/postmaster/pgstat.c
+++ b/src/backend/postmaster/pgstat.c
@@ -3340,8 +3340,8 @@ pgstat_get_wait_client(WaitEventClient w)
 		case WAIT_EVENT_WAL_RECEIVER_WAIT_START:
 			event_name = "WalReceiverWaitStart";
 			break;
-		case WAIT_EVENT_LIBPQWALRECEIVER_READ:
-			event_name = "LibPQWalReceiverRead";
+		case WAIT_EVENT_LIBPQWALRECEIVER:
+			event_name = "LibPQWalReceiver";
 			break;
 		case WAIT_EVENT_WAL_SENDER_WAIT_WAL:
 			event_name = "WalSenderWaitForWAL";
diff --git a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
index 7df3698..8dc51d4 100644
--- a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
+++ b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
@@ -112,6 +112,7 @@ libpqrcv_connect(const char *conninfo, bool logical, const char *appname,
  char **err)
 {
 	WalReceiverConn *conn;
+	PostgresPollingStatusType status;
 	const char *keys[5];
 	const char *vals[5];
 	int			i = 0;
@@ -138,7 +139,60 @@ libpqrcv_connect(const char *conninfo, bool logical, const char *appname,
 	vals[i] = NULL;
 
 	conn = palloc0(sizeof(WalReceiverConn));
-	conn->streamConn = PQconnectdbParams(keys, vals, /* expand_dbname = */ true);
+	conn->streamConn = PQconnectStartParams(keys, vals,
+			/* expand_dbname = */ true);
+	/* Check for conn status. */
+	if (PQstatus(conn->streamConn) == CONNECTION_BAD)
+	{
+		*err = pstrdup(PQerrorMessage(conn->streamConn));
+		return NULL;
+	}
+
+	/* Poll connection. */
+	do
+	{
+		int			rc;
+		int			extra_flag;
+
+		/* Determine which function we should use for Polling. */
+		status = PQconnectPoll(conn->streamConn);
+
+		/* Next action based upon status value. */
+		switch (status)
+		{
+			case PGRES_POLLING_READING:
+extra_flag = WL_SOCKET_READABLE;
+/* pass through */
+			case PGRES_POLLING_WRITING:
+extra_flag = WL_SOCKET_WRITEABLE;
+
+ResetLatch(>procLatch);
+rc = WaitLatchOrSocket(>procLatch,
+	   WL_POSTMASTER_DEATH |
+	   WL_LATCH_SET | extra_flag,
+	   PQsocket(conn->streamConn),
+	   0,
+	   

Re: [HACKERS] Valgrind-detected bug in partitioning code

[Tom Lane]

Stephen Frost  writes:
> * Tom Lane (t...@sss.pgh.pa.us) wrote:
>> Robert Haas  writes:
>>> Hmm.  That's bad.  I kind of wonder how sane it is to think that we
>>> can invoke SQL-callable functions during a relcache reload,

>> You're doing WHAT?

> Uh.  +1.

Now that I've calmed down a bit: the right way to do this sort of thing is
simply to flush the invalidated data during reload, and recompute it when
it is next requested, which necessarily will be inside a valid
transaction.  Compare e.g. the handling of the lists of a relation's
indexes.

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] Declarative partitioning - another take

[Robert Haas]

On Fri, Jan 20, 2017 at 1:15 AM, Andres Freund  wrote:
> On 2017-01-19 14:18:23 -0500, Robert Haas wrote:
>> Committed.
>
> One of the patches around this topic committed recently seems to cause
> valgrind failures like
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=skink=2017-01-19%2008%3A40%3A02
> :

Tom Lane independently reported this same issue.  I believe I've fixed
it.  Sorry for not noticing and crediting this report also.

-- 
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] Too many autovacuum workers spawned during forced auto-vacuum

[Robert Haas]

On Fri, Jan 20, 2017 at 2:43 AM, Michael Paquier
 wrote:
> On Fri, Jan 20, 2017 at 4:11 AM, Alvaro Herrera
>  wrote:
>> Robert Haas wrote:
>>
>>> After sleeping on this, I'm inclined to go with Amit's fix for now.
>>> It seems less likely to break anything in the back-branches than any
>>> other option I can think up.
>>
>> Yeah, no objections here.
>
> +1.

OK, committed and back-patched all the way.

-- 
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] Vacuum: allow usage of more than 1GB of work mem

[Alvaro Herrera]

You posted two patches with this preamble:

Claudio Freire wrote:

> Attached is the raw output of the test, the script used to create it,
> and just in case the patch set used. I believe it's the same as the
> last one I posted, just rebased.

There was no discussion whatsoever of the "prefetch" patch in this
thread; and as far as I can see, nobody even mentioned such an idea in
the thread.  This prefetch patch appeared out of the blue and there was
no discussion about it that I can see.  Now I was about to push it after
some minor tweaks, and went to search where was its justification, only
to see that there was none.  Did anybody run tests with this patch?

I attach it now one more time.  My version is based on the latest
Claudio posted at
https://postgr.es/m/CAGTBQpa464RugxYwxLTtDi=syv9gngfcjk8uzb2fr6nddqu...@mail.gmail.com
I don't know if there are differences to the version first posted.
I only changed the magic number 32 to a #define, and added a
CHECK_FOR_INTERRUPTS in the prefetching loop.

FWIW, I think this patch is completely separate from the maint_work_mem
patch and should have had its own thread and its own commitfest entry.
I intend to get a look at the other patch next week, after pushing this
one.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>From bd10f56beb365e3cbfadb87d27f8aeb4b33f880e Mon Sep 17 00:00:00 2001
From: Alvaro Herrera 
Date: Fri, 20 Jan 2017 18:04:51 -0300
Subject: [PATCH] Prefetch blocks during lazy vacuum's truncation scan

The truncation scan can be sped up on rotating media by prefetching
blocks in forward direction, because the is a cue for the operating
system's readahead to kick in, so that by the time we request those
blocks, they are already in memory.

Author: Claudio Freire
Discussion: 
https://postgr.es/m/cagtbqpa6nfgo_6g_y_7zqx8l9gchdsqkydo1tguh791z6py...@mail.gmail.com
---
 src/backend/commands/vacuumlazy.c | 36 ++--
 1 file changed, 34 insertions(+), 2 deletions(-)

diff --git a/src/backend/commands/vacuumlazy.c 
b/src/backend/commands/vacuumlazy.c
index a2999b3..e676072 100644
--- a/src/backend/commands/vacuumlazy.c
+++ b/src/backend/commands/vacuumlazy.c
@@ -98,6 +98,12 @@
  */
 #define SKIP_PAGES_THRESHOLD   ((BlockNumber) 32)
 
+/*
+ * Size of the prefetch window for lazy vacuum backwards truncation scan.
+ * Needs to be a power of 2.
+ */
+#define PREFETCH_SIZE  32
+
 typedef struct LVRelStats
 {
/* hasindex = true means two-pass strategy; false means one-pass */
@@ -1825,14 +1831,24 @@ lazy_truncate_heap(Relation onerel, LVRelStats 
*vacrelstats)
 static BlockNumber
 count_nondeletable_pages(Relation onerel, LVRelStats *vacrelstats)
 {
-   BlockNumber blkno;
+   BlockNumber blkno,
+   prefetchBlkno;
instr_time  starttime;
 
/* Initialize the starttime if we check for conflicting lock requests */
INSTR_TIME_SET_CURRENT(starttime);
 
-   /* Strange coding of loop control is needed because blkno is unsigned */
+   /*
+* Start checking blocks at what we believe relation end to be and move
+* backwards.  (Strange coding of loop control is needed because blkno 
is
+* unsigned.)  To make it a bit faster, we prefetch a bunch of blocks 
at a
+* time in forward direction, so that OS-level readahead can kick in to
+* speed this up.
+*/
blkno = vacrelstats->rel_pages;
+   prefetchBlkno = blkno & ~(PREFETCH_SIZE - 1);
+   prefetchBlkno =
+   (prefetchBlkno > PREFETCH_SIZE) ? prefetchBlkno - PREFETCH_SIZE 
: 0;
while (blkno > vacrelstats->nonempty_pages)
{
Buffer  buf;
@@ -1882,6 +1898,22 @@ count_nondeletable_pages(Relation onerel, LVRelStats 
*vacrelstats)
 
blkno--;
 
+   /* If we haven't prefetched this lot yet, do so now. */
+   if (blkno <= prefetchBlkno)
+   {
+   BlockNumber pblkno;
+
+   if (prefetchBlkno >= PREFETCH_SIZE)
+   prefetchBlkno -= PREFETCH_SIZE;
+   else
+   prefetchBlkno = 0;
+   for (pblkno = prefetchBlkno; pblkno < blkno; pblkno++)
+   {
+   PrefetchBuffer(onerel, MAIN_FORKNUM, pblkno);
+   CHECK_FOR_INTERRUPTS();
+   }
+   }
+
buf = ReadBufferExtended(onerel, MAIN_FORKNUM, blkno,
 RBM_NORMAL, 
vac_strategy);
 
-- 
2.1.4


-- 
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] Valgrind-detected bug in partitioning code

[Stephen Frost]

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Robert Haas  writes:
> > Hmm.  That's bad.  I kind of wonder how sane it is to think that we
> > can invoke SQL-callable functions during a relcache reload, because
> > couldn't we be processing an invalidation in the context of an aborted
> > transaction?
> 
> You're doing WHAT?

Uh.  +1.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Valgrind-detected bug in partitioning code

[Tom Lane]

Robert Haas  writes:
> Hmm.  That's bad.  I kind of wonder how sane it is to think that we
> can invoke SQL-callable functions during a relcache reload, because
> couldn't we be processing an invalidation in the context of an aborted
> transaction?

You're doing WHAT?

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] pg_hba_file_settings view patch

[Tom Lane]

Haribabu Kommi  writes:
> [ pg_hba_rules_10.patch ]

I took a quick look over this.

* I'm not exactly convinced that the way you approached the error message
reporting, ie duplicating the logged message, is good.  In particular
this results in localizing the strings reported in pg_hba_rules.error,
which is exactly opposite to the decision we reached for the
pg_file_settings view.  What's the reasoning for deciding that this
view should contain localized strings?  (More generally, we found in
the pg_file_settings view that we didn't always want to use exactly
the same string that was logged, anyway.)

* Also, there seems to be a lot of ereports remaining unconverted,
eg the "authentication file token too long" error.  One of the things
we wanted pg_file_settings to be able to do was finger pretty much any
mistake in the config file, including syntax errors.  It seems like
it'd be a shame if pg_hba_rules is unable to help with that.  You
should be able to fill in line number and error even if the line is
too mangled to be able to populate the other fields sanely.

* While we're on the comparison to pg_file_settings ... pg_hba_rules
is not the view name I'd guess if I guessed one based on that precedent.
I don't have a better suggestion offhand, but this name seems weirdly
inconsistent.

* I think "memcxt" as a field name is pretty unhelpful if you suppose
it just means "memory context", and outright misleading if you guess
it's, say, the context the tuplestore is in.  Perhaps call it "tmpcxt"
and add a comment like "Short-lived context, reset after each line".
The other fields of FillHbaLineCxt could do with comments too.

* ... although really, you've gone way overboard with temp contexts
here.  I don't think it's necessary to have a per-line context at all;
you could just do all the work in the single temp context that fill_hba
calls hbacxt, and drop it all at end of function, because no matter what
you'll be eating O(file size) space, and we're just quibbling over the
size of the multiplier.  Also, if you're concerned with reclaiming space
before end of query, aren't you leaking the tokenize_file output data?

* getauthmethod() might be better replaced with an array.  And doesn't it
produce uninitialized-variable warnings for you?

* It seems a little weird that fill_hba_auth_opt isn't inserting the "="
between name and value.  And should it be using psprintf?  It's the
only use of StringInfo in this file, so it looks a bit out of place.
Actually, I wonder if you wouldn't be better off replacing it with a
coding style like

options[noptions++] =
CStringGetTextDatum(psprintf("ldapport=%d", hba->ldapport));

which seems more readable and more flexible.

* "MAX_OPTIONS" is, uh, mighty generic.  Maybe "MAX_HBA_OPTIONS"?
And the comment for it doesn't actually tell you what it is.

* NUM_PG_HBA_LOOKUP_ATTS seems like it ought to match the name of the
view, ie NUM_PG_HBA_RULES_ATTS if that doesn't get renamed.

* Usually we just write "if (listvar)" or "if (listvar != NIL)" rather
than "if (list_length(listvar) != 0)".  list_length() overspecifies what
you need to test.  This isn't as critical as it was back in the day when
list_length() cost O(N), but still the former is much more common project
style.

* Why is AllocateFile() failure only an ereport(LOG) in fill_hba()?
>From the user's viewpoint he'll get an empty view with no visible
reason.  Probably ereport(ERROR) is more sensible.  You could imagine
trying to show the error in the view, but that seems like more work
than the case warrants.

* Seems like the FillHbaLineCxt variable could just be a local struct
in hba_rules(), and dispense with one palloc/pfree cycle.

* I'm not really on board with patches modifying pgindent/typedefs.list
retail.  To my mind that file represents the typedefs used the last
time we pgindent'd the whole tree, and if you want an up-to-date list
you should ask the buildfarm.  Otherwise there's just too much confusion
stemming from the fact that not everybody updates it when patching.

My own workflow for reindenting patches goes more like
curl https://buildfarm.postgresql.org/cgi-bin/typedefs.pl -o my-typedefs.list
... manually edit my-typedefs.list to add any new typedefs from patch ...
pgindent --typedefs=my-typedefs.list target-files

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] Valgrind-detected bug in partitioning code

[Robert Haas]

On Fri, Jan 20, 2017 at 8:09 AM, Tom Lane  wrote:
> skink has been unhappy since commit d26fa4f went in, but I think
> that just exposed a pre-existing bug.  Running valgrind here
> duplicates the failure:
>
> ==00:00:02:01.653 16626== Conditional jump or move depends on uninitialised 
> value(s)
> ==00:00:02:01.653 16626==at 0x4BDF6B: btint4cmp (nbtcompare.c:97)
> ==00:00:02:01.653 16626==by 0x81D6BE: FunctionCall2Coll (fmgr.c:1318)
> ==00:00:02:01.653 16626==by 0x52D584: partition_bounds_equal 
> (partition.c:627)
> ==00:00:02:01.653 16626==by 0x80CF8E: RelationClearRelation 
> (relcache.c:1203)
> ==00:00:02:01.653 16626==by 0x80E601: RelationCacheInvalidateEntry 
> (relcache.c:2662)
> ==00:00:02:01.653 16626==by 0x803DD6: LocalExecuteInvalidationMessage 
> (inval.c:568)
> ==00:00:02:01.653 16626==by 0x803F53: ProcessInvalidationMessages.clone.0 
> (inval.c:444)
> ==00:00:02:01.653 16626==by 0x8040C8: CommandEndInvalidationMessages 
> (inval.c:1056)
> ==00:00:02:01.653 16626==by 0x80C719: RelationSetNewRelfilenode 
> (relcache.c:3490)
> ==00:00:02:01.653 16626==by 0x5CD50A: ExecuteTruncate (tablecmds.c:1393)
> ==00:00:02:01.653 16626==by 0x721AC7: standard_ProcessUtility 
> (utility.c:532)
> ==00:00:02:01.653 16626==by 0x71D943: PortalRunUtility (pquery.c:1163)
>
> IOW, partition_bounds_equal() is testing uninitialized memory during
> a TRUNCATE on a partitioned table.

Hmm.  That's bad.  I kind of wonder how sane it is to think that we
can invoke SQL-callable functions during a relcache reload, because
couldn't we be processing an invalidation in the context of an aborted
transaction?  And I wonder why we really need or want to do that
anyway.  For purposes of equalPartitionDescs(), it seems like the
relevant test is datumIsEqual(), not the equality operator derived
from the partition opclass.

But I think the immediate problem here is some fuzzy thinking about
the handling of the values taken from b1->content and b2->content.
Those have to be checked before examining values from b1->datums
and/or b2->datums, and the latter should be inspected only if the
former are both identical and both RANGE_DATUM_FINITE.  I'll push a
fix for that.

-- 
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] Microvacuum support for Hash Index

[Jesper Pedersen]

Hi Ashutosh,

On 01/20/2017 04:18 AM, Ashutosh Sharma wrote:

okay, Thanks for confirming that.

I would like to update you that I am not able to reproduce this issue
at my end. I suspect that the steps i am following might be slightly
different than your's. Could you please have a look at steps mentioned
below and confirm if there is something different that I am doing.

Firstly, I am running the test-case on following git commit in head:


commit ba61a04bc7fefeee03416d9911eb825c4897c223
Author: Tom Lane 
Date:   Thu Jan 19 19:52:13 2017 -0500

Avoid core dump for empty prepared statement in an aborted transaction.

Brown-paper-bag bug in commit ab1f0c822: the old code here coped with
null CachedPlanSource.raw_parse_tree, the new code not so much.
Per report from Dave Cramer.


On top of above commit, I have applied WAL v8 patch for hash index and
MV v5 patch.

Now, with an --enable-cassert build I am following below steps:

1) Created a 'test' database

2) psql -d test -f ~/ddl.sql

where ddl.sql is,

-- ddl.sql --
CREATE TABLE test AS SELECT generate_series(1, 10) AS id, 0 AS val;
CREATE INDEX IF NOT EXISTS idx_id ON test USING hash (id);
CREATE INDEX IF NOT EXISTS idx_val ON test USING hash (val);
ANALYZE;
-- ddl.sql --

3) pgbench -M prepared -c 10 -j 10 -T 1800 -f ~/test.sql test

where test.sql is,

-- test.sql --
\set id random(1,10)
\set val random(0,10)
BEGIN;
UPDATE test SET val = :val WHERE id = :id;
COMMIT;
-- test.sql --


Machine details are as follows:

Architecture:  x86_64
CPU op-mode(s):32-bit, 64-bit
Byte Order:Little Endian
CPU(s):128
On-line CPU(s) list:   0-127
Thread(s) per core:2
Core(s) per socket:8
Socket(s): 8

Also, It would be great if you could confirm as if you have been
getting this issue repeatedly. Thanks.



Yeah, those are the steps; just with a Skylake laptop.

However, I restarted with a fresh master, with WAL v8 and MV v5, and 
can't reproduce the issue.


Best regards,
 Jesper



--
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] delta relations in AFTER triggers

[Nico Williams]

On Fri, Jan 20, 2017 at 01:37:33PM -0600, Kevin Grittner wrote:
> On Thu, Jan 19, 2017 at 4:14 PM, Nico Williams  wrote:
> 
> Reviews welcome!

I will review.

> There is currently plenty of room for pseudo-MV implementations,
> and may be for a while.  It's a good indication of the need for the
> feature in core.  An implementation in the guts of core can have
> advantages that nothing else can, of course.  For example, for
> eager application of the deltas, nothing will be able to beat
> capturing tuples already in RAM and being looked at for possible
> trigger firing into a RAM-with-spill-to-disk tuplestore.

BTW, automatic updates of certain types of MVs should be easy: add
constraints based on NEW/OLD rows from synthetic triggers to the
underlying query.  However, there is a bug in the query planner that
prevents this from being very fast.  At some point I want to tackle that
bug.  Basically, the planner does not notice that a table source in a
join has a lookup key sufficiently well-specified by those additional
constraints that it should be the first table source in the outermost
loop.

> I don't have time to review what you've done right now, but will
> save that link to look at later, if you give permission to borrow
> from it (with proper attribution, of course) if there is something
> that can advance what I'm doing.  If such permission is not
> forthcoming, I will probably avoid looking at it, to avoid any
> possible copyright issues.

Our intention is to contribute this.  We're willing to sign reasonable
contribution agreements.

I'd appreciate a review, for sure.  Thanks!

> > Incidentally, it's really nice that PG has some "higher order" SQL
> > features that make this sort of thing easier.  In particular, here, row
> > values and record types, and being able to refer to a table as a column
> > of the table's record type.
> 
> Yeah, I found that quite handy in developing the REFRESH feature,
> and expect to be using it in incremental maintenance.

Indeed, I *copied* the pattern.  However, I didn't have to generate
SELECT statements that include column names, as I was able to just
compare table source row values.  There's a gotcha w.r.t. NULL columns,
but it affects the built-in REFRESH as well, IIRC.  The commentary in
our implementation discusses that in more detail.

Nico
-- 


-- 
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: Grouped base relation

[Robert Haas]

On Thu, Jan 19, 2017 at 2:19 AM, Tomas Vondra
 wrote:
>> Not all aggregates have TransValue * integer = newTransValue
>> behaviour. Example is array_agg() or string_agg() has "TransValue
>> concatenated integer time" behaviour. Or an aggregate "multiplying"
>> values across rows will have TransValue (raised to) integer behaviour.
>> Labelling all of those as "multi" doesn't look good.
>
> All aggregates that have (or can have) a combine function have it, because
> in the worst case you can simply implement it by calling the combine
> function repeatedly.

+1.

> Also, if you treat the combine function as "+" then the "multiply" function
> is exactly what "*" is expected to do. So I find the naming quite
> appropriate, actually.

+1.

> But I think naming of the function is not the most important aspect of the
> patch, I believe. In the worst case, we can do s/multi/whatever/ sometime
> later.

Yeah.

-- 
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] Logical Replication WIP

[Robert Haas]

On Fri, Jan 20, 2017 at 2:57 PM, Craig Ringer  wrote:
> > I don't see how a subscription can do anything useful with wal_level <
> > logical?
>
> The upstream must have it set to logical so we can decide the change stream.
>
> The downstream need not. It's an independent instance.

/me facepalms.

Thanks.

-- 
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] Possible issue with expanded object infrastructure on Postgres 9.6.1

[Robert Haas]

On Thu, Jan 19, 2017 at 9:39 PM, Tom Lane  wrote:
> If I had to bet on the basis of this much info, I would bet that the
> parallel-query infrastructure is dropping the ball somewhere and
> transmitting a corrupted datum that accidentally looks like it is
> an expanded-object reference.

There's code in datumSerialize() that is intended to do the right
thing with expanded-object references.  But it could have a bug, of
course.  That function gets called from SerializeParamList(), which
might be another place to look for problems if the crash can only be
reproduced when there are statement parameters.

-- 
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] Logical Replication WIP

[Craig Ringer]

On 21 Jan. 2017 06:48, "Robert Haas"  wrote:

On Fri, Jan 20, 2017 at 11:39 AM, Petr Jelinek
 wrote:
> Launcher is needed for subscriptions, subscriptions don't depend on
> wal_level.

I don't see how a subscription can do anything useful with wal_level <
logical?


The upstream must have it set to logical so we can decide the change stream.

The downstream need not. It's an independent instance.

Re: [HACKERS] increasing the default WAL segment size

[Robert Haas]

On Fri, Jan 20, 2017 at 2:34 AM, Michael Paquier
 wrote:
> On Fri, Jan 20, 2017 at 12:49 AM, Robert Haas  wrote:
>> On Wed, Jan 18, 2017 at 12:42 PM, Robert Haas  wrote:
>>> Problems 2-4 actually have to do with a DestReceiver of type
>>> DestRemote really, really wanting to have an associated Portal and
>>> database connection, so one approach is to create a stripped-down
>>> DestReceiver that doesn't care about those things and then passing
>>> that to GetPGVariable.
>>
>> I tried that and it worked out pretty well, so I'm inclined to go with
>> this approach.  Proposed patches attached.  0001 adds the new
>> DestReceiver type, and 0002 is a revised patch to implement the SHOW
>> command itself.
>>
>> Thoughts, comments?
>
> This looks like a sensible approach to me. DestRemoteSimple could be
> useful for background workers that are not connected to a database as
> well. Isn't there a problem with PGC_REAL parameters?

No, because the output of SHOW is always of type text, regardless of
the type of the GUC.

-- 
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] Logical Replication WIP

[Robert Haas]

On Fri, Jan 20, 2017 at 11:39 AM, Petr Jelinek
 wrote:
> Launcher is needed for subscriptions, subscriptions don't depend on
> wal_level.

I don't see how a subscription can do anything useful with wal_level < logical?

-- 
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] Parallel Index Scans

[Robert Haas]

On Fri, Jan 20, 2017 at 9:29 AM, Amit Kapila  wrote:
> Sure, if scan keys have changed then we can't continue, but this is
> the case where runtime keys are first time initialized.
>
> if (node->iss_NumRuntimeKeys != 0 && !node->iss_RuntimeKeysReady)
>
> In the above check, the second part of the check
> (!node->iss_RuntimeKeysReady) ensures that it is for the first time.
> Now, let me give you an example to explain what bad can happen if we
> allow resetting parallel scan in this case.  Consider a query like
> select * from t1 where c1 < parallel_index(10);, in this if we allow
> resetting parallel scan descriptor during first time initialization of
> runtime keys, it can easily corrupt the parallel scan state.  Suppose
> leader has taken the lead and is scanning some page and worker reaches
> to initialize its keys in ExecReScanIndexScan(), if worker resets the
> parallel scan, then it will corrupt the state of the parallel scan
> state.

Hmm, I see.  So the problem if I understand it correctly is that every
participating process needs to update the backend-private state for
the runtime keys and only one of those processes can update the shared
state.  But in the case of a "real" rescan, even the shared state
needs to be reset.  OK, makes sense.

Why does btparallelrescan cater to the case where scan->parallel_scan
== NULL?  I would assume it should never get called in that case.
Also, I think ExecReScanIndexScan needs significantly better comments.
After some thought I see what's it's doing - mostly anyway - but I was
quite confused at first.  I still don't completely understand why it
needs this if-test:

+   /* reset (parallel) index scan */
+   if (node->iss_ScanDesc)
+   {

>> I extracted the generic portions of this infrastructure (i.e. not the
>> btree-specific stuff) and spent some time working on it today.  The
>> big thing I fixed was the documentation, which you added in a fairly
>> illogical part of the file.
>
> Hmm, it is not illogical. All the functions are described in the same
> order as they are declared in IndexAmRoutine structure and I have
> followed the same.

I see.  Sorry, I didn't realize that was what you were going for.

> I think both amestimateparallelscan and
> aminitparallelscan should be added one para down which says (The
> purpose of an index .. The scan-related functions that an index access
> method must or may provide are:).

I think it's a good idea to put all three of those functions together
in the listing, similar to what we did in
69d34408e5e7adcef8ef2f4e9c4f2919637e9a06 for FDWs.  After all they are
closely related in purpose, and it may be easiest to understand if
they are next to each other in the listing.  I suggest that we move
them to the end in IndexAmRoutine similar to the way FdwRoutine was
done; in other words, my idea is to make the structure consistent with
the way that I revised the documentation instead of making the
documentation consistent with the order you picked for the structure
members.  What I like about that is that it gives a good opportunity
to include some general remarks on parallel index scans in a central
place, as I did in that patch.  Also, it makes it easier for people
who care about parallel index scans to find all of the related things
(since they are together) and for people who don't care about them to
ignore it all (for the same reason).  What do you think about that
approach?

-- 
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] delta relations in AFTER triggers

[Kevin Grittner]

On Thu, Jan 19, 2017 at 4:14 PM, Nico Williams  wrote:

> I hope what I've done about delta relations will be mostly irrelevant
> given your patch (which I've not looked at in detail),

Reviews welcome!

> but just FYI,
> I've built an alternate, all-SQL-coded materialized view system that
> captures deltas between refreshes and deltas from direct DMLs of the
> materialized view:

There is currently plenty of room for pseudo-MV implementations,
and may be for a while.  It's a good indication of the need for the
feature in core.  An implementation in the guts of core can have
advantages that nothing else can, of course.  For example, for
eager application of the deltas, nothing will be able to beat
capturing tuples already in RAM and being looked at for possible
trigger firing into a RAM-with-spill-to-disk tuplestore.

> https://github.com/twosigma/postgresql-contrib/blob/master/pseudo_mat_views.sql
>
> There are some good ideas there, IMO, even if that implementation were
> useless because of your patch.

I don't have time to review what you've done right now, but will
save that link to look at later, if you give permission to borrow
from it (with proper attribution, of course) if there is something
that can advance what I'm doing.  If such permission is not
forthcoming, I will probably avoid looking at it, to avoid any
possible copyright issues.

> Incidentally, it's really nice that PG has some "higher order" SQL
> features that make this sort of thing easier.  In particular, here, row
> values and record types, and being able to refer to a table as a column
> of the table's record type.

Yeah, I found that quite handy in developing the REFRESH feature,
and expect to be using it in incremental maintenance.

--
Kevin Grittner
EDB: 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] delta relations in AFTER triggers

[Kevin Grittner]

Attached is v10 which fixes bitrot from v9 caused by ea15e186.

Still needs review.

-- 
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


transition-v10.diff.gz
Description: GNU Zip compressed data

-- 
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] Failure in commit_ts tap tests

[Tom Lane]

Alvaro Herrera  writes:
> Hmm.  The test works fine for me, even if it should be obvious that the
> use of the != operator is wrong.  I don't understand why it causes a
> failure only for you.

Even more interesting, the warning appears as-expected in stripped down
test cases, eg

$ perl -e 'use warnings; use Test::More; ok("Foo" ne "bar", "ok");'
ok 1 - ok
# Tests were run but no plan was declared and done_testing() was not seen.

$ perl -e 'use warnings; use Test::More; ok("Foo" != "bar", "ok");'
Argument "bar" isn't numeric in numeric ne (!=) at -e line 1.
Argument "Foo" isn't numeric in numeric ne (!=) at -e line 1.
not ok 1 - ok
#   Failed test 'ok'
#   at -e line 1.
# Tests were run but no plan was declared and done_testing() was not seen.

I really don't understand why this complains but the same perl version
is happy with (the previous coding in) 004_restart.pl.  Perl bug?

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] Failure in commit_ts tap tests

[Alvaro Herrera]

Pavan Deolasee wrote:

> Argument "" isn't numeric in numeric ne (!=) at t/004_restart.pl line 57.
> Argument "Fri Jan 20 07:59:52.322811 2017 PST" isn't numeric in numeric ne 
> (!=) at t/004_restart.pl line 57.
> not ok 8 - commit timestamp recorded
> 
> Changing the operator to "ne" works for me (patch attached). But I wonder
> if this is something specific to my system? Am I using a wrong/stale
> version on OSX Sierra? I'm surprised nobody reported this problem earlier.

Hmm.  The test works fine for me, even if it should be obvious that the
use of the != operator is wrong.  I don't understand why it causes a
failure only for you.

> $ perl -v

> This is perl 5, version 18, subversion 2 (v5.18.2) built for
> darwin-thread-multi-2level
> (with 2 registered patches, see perl -V for more detail)

Mine says:

This is perl 5, version 20, subversion 2 (v5.20.2) built for 
x86_64-linux-gnu-thread-multi
(with 92 registered patches, see perl -V for more detail)

-- 
Álvaro Herrerahttps://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] Failure in commit_ts tap tests

[Tom Lane]

Pavan Deolasee  writes:
> I'm surprised nobody reported this problem earlier.

It looks like at least part of the answer is that the buildfarm isn't
running this test.  AFAICS, it runs "make installcheck" not
"make check" in src/test/modules.  I don't know whether any of the
critters would have duplicated the failure, but they weren't trying.

I don't know if any of the other src/test/modules/ subdirectories
test more in make check mode than make installcheck mode, but if
they do, we probably oughta change the buildfarm script.

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] Failure in commit_ts tap tests

[Tom Lane]

Pavan Deolasee  writes:
> I see a consistent failure while running "make -C
> src/test/modules/commit_ts/ check" on a server compiled with
> --enable-tap-tests. This is on the master branch (193a7d791)
> ...
> Changing the operator to "ne" works for me (patch attached). But I wonder
> if this is something specific to my system? Am I using a wrong/stale
> version on OSX Sierra? I'm surprised nobody reported this problem earlier.

That's just weird.  I agree this patch is correct (and I see Alvaro
just pushed it), but why isn't the code failing elsewhere?  I see
no such warning on my RHEL6 box (perl 5.10.1) nor on my OSX Sierra
laptop (perl 5.18.2, should be same as yours).  Do you have any
nondefault Perl modules installed?

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] Logical Replication WIP

[Jaime Casanova]

On 20 January 2017 at 11:39, Petr Jelinek  wrote:
> On 20/01/17 17:33, Jaime Casanova wrote:
>>>
>>
>> surely wal_level < logical shouldn't start a logical replication
>> launcher, and after an initdb wal_level is only replica
>>
>
> Launcher is needed for subscriptions, subscriptions don't depend on
> wal_level.
>

mmm... ok, i need to read a little then. thanks

-- 
Jaime Casanova  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

[HACKERS] Failure in commit_ts tap tests

[Pavan Deolasee]

I see a consistent failure while running "make -C
src/test/modules/commit_ts/ check" on a server compiled with
--enable-tap-tests. This is on the master branch (193a7d791)

t/004_restart.pl 
1..16
ok 1 - getting ts of InvalidTransactionId reports error
ok 2 - expected error from InvalidTransactionId
ok 3 - getting ts of BootstrapTransactionId succeeds
ok 4 - timestamp of BootstrapTransactionId is null
ok 5 - getting ts of FrozenTransactionId succeeds
ok 6 - timestamp of FrozenTransactionId is null
ok 7 - committs for FirstNormalTransactionId is null
not ok 8 - commit timestamp recorded

Upon investigating logs, I see that perl complains about using != operator
on non-numeric argument.

Argument "" isn't numeric in numeric ne (!=) at t/004_restart.pl line 57.
Argument "Fri Jan 20 07:59:52.322811 2017 PST" isn't numeric in numeric ne
(!=) at t/004_restart.pl line 57.
not ok 8 - commit timestamp recorded

Changing the operator to "ne" works for me (patch attached). But I wonder
if this is something specific to my system? Am I using a wrong/stale
version on OSX Sierra? I'm surprised nobody reported this problem earlier.

$ perl -v

This is perl 5, version 18, subversion 2 (v5.18.2) built for
darwin-thread-multi-2level
(with 2 registered patches, see perl -V for more detail)

Copyright 1987-2013, Larry Wall

Thanks,
Pavan

-- 
 Pavan Deolasee   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services


commit_ts_taptest.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

Re: [HACKERS] pgbench more operators & functions

[Fabien COELHO]

Rebased version after small expression scanner change.

--
Fabien.diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml
index 1eee8dc..8bb9c75 100644
--- a/doc/src/sgml/ref/pgbench.sgml
+++ b/doc/src/sgml/ref/pgbench.sgml
@@ -828,11 +828,11 @@ pgbench  options  dbname
   The expression may contain integer constants such as 5432,
   double constants such as 3.14159,
   references to variables :variablename,
-  unary operators (+, -) and binary operators
-  (+, -, *, /,
-  %) with their usual precedence and associativity,
-  function calls, and
-  parentheses.
+  operators
+  with their usual precedence and associativity,
+  function calls,
+  SQL CASE generic conditional expressions
+  and parentheses.
  
 
  
@@ -917,6 +917,165 @@ pgbench  options  dbname
   
  
 
+ 
+  Built-In Operators
+
+  
+   The arithmetic, bitwise, comparison and logical operators listed in
+are built into pgbench
+   and may be used in expressions appearing in
+   \set.
+  
+
+  
+   pgbench Operators by increasing precedence
+   
+
+ 
+  Operator
+  Description
+  Example
+  Result
+ 
+
+
+ 
+  or
+  logical or
+  5 or 0
+  1
+ 
+ 
+  and
+  logical and
+  3 and 0
+  0
+ 
+ 
+  |
+  integer bitwise OR
+  1 | 2
+  3
+ 
+ 
+  #
+  integer bitwise XOR
+  1 # 3
+  2
+ 
+ 
+  
+  integer bitwise AND
+  1  3
+  1
+ 
+ 
+  =
+  is equal
+  5 = 4
+  0
+ 
+ 
+  
+  is not equal
+  5  4
+  1
+ 
+ 
+  !=
+  is not equal
+  5 != 5
+  0
+ 
+ 
+  
+  lower than
+  5  4
+  0
+ 
+ 
+  =
+  lower or equal
+  5 = 4
+  0
+ 
+ 
+  
+  greater than
+  5  4
+  1
+ 
+ 
+  =
+  greater or equal
+  5 = 4
+  1
+ 
+ 
+  
+  left shift
+  1  2
+  4
+ 
+ 
+  
+  right shift
+  8  2
+  2
+ 
+ 
+  +
+  addition
+  5 + 4
+  9
+ 
+ 
+  -
+  substraction
+  3 - 2.0
+  1.0
+ 
+ 
+  *
+  multiplication
+  5 * 4
+  20
+ 
+ 
+  /
+  division (integer truncates the results)
+  5 / 3
+  1
+ 
+ 
+  %
+  modulo
+  3 % 2
+  1
+ 
+ 
+  not
+  logical NOT
+  not 7
+  0
+ 
+ 
+  ~
+  bitwise NOT
+  ~ 1
+  -2
+ 
+ 
+  -
+  opposite
+  - 2.0
+  -2.0
+ 
+
+   
+  
+ 
+
  
   Built-In Functions
 
@@ -963,6 +1122,13 @@ pgbench  options  dbname
5432.0
   
   
+   exp(x)
+   double
+   exponential
+   exp(1.0)
+   2.718281828459045
+  
+  
greatest(a [, ... ] )
double if any a is double, else integer
largest value among arguments
@@ -970,6 +1136,13 @@ pgbench  options  dbname
5
   
   
+   if(c,e1,e2)
+   same as e*
+   if c is not zero then e1 else e2
+   if(0,1.0,2.0)
+   2.0
+  
+  
int(x)
integer
cast to int
@@ -984,6 +1157,13 @@ pgbench  options  dbname
2.1
   
   
+   ln(x)
+   double
+   natural logarithm
+   ln(2.718281828459045)
+   1.0
+  
+  
pi()
double
value of the constant PI
diff --git a/src/bin/pgbench/exprparse.y b/src/bin/pgbench/exprparse.y
index b3a2d9b..0055e51 100644
--- a/src/bin/pgbench/exprparse.y
+++ b/src/bin/pgbench/exprparse.y
@@ -24,8 +24,10 @@ static PgBenchExpr *make_double_constant(double dval);
 static PgBenchExpr *make_variable(char *varname);
 static PgBenchExpr *make_op(yyscan_t yyscanner, const char *operator,
 		PgBenchExpr *lexpr, PgBenchExpr *rexpr);
+static PgBenchExpr *make_uop(yyscan_t yyscanner, const char *operator, PgBenchExpr *expr);
 static int	find_func(yyscan_t yyscanner, const char *fname);
 static PgBenchExpr *make_func(yyscan_t yyscanner, int fnumber, PgBenchExprList *args);
+static PgBenchExpr *make_case(yyscan_t yyscanner, PgBenchExprList *when_then_list, PgBenchExpr *else_part);
 
 %}
 
@@ -45,18 +47,28 @@ static PgBenchExpr *make_func(yyscan_t yyscanner, int fnumber, PgBenchExprList *
 	PgBenchExprList *elist;
 }
 
-%type  elist
-%type  expr
+%type  elist when_then_list
+%type  expr case_control
 %type  INTEGER_CONST function
 %type  DOUBLE_CONST
 %type  VARIABLE FUNCTION
 
 %token INTEGER_CONST DOUBLE_CONST VARIABLE FUNCTION
-
-/* Precedence: lowest to highest */
+%token AND_OP OR_OP NE_OP LE_OP GE_OP LS_OP RS_OP
+%token CASE_KW WHEN_KW THEN_KW ELSE_KW END_KW
+
+/* Precedence: lowest to highest, taken from C */
+%left	OR_OP
+%left	AND_OP
+%left   '|'
+%left   '#'
+%left   '&'
+%left	'=' NE_OP
+%left	'<' '>' LE_OP GE_OP
+%left	LS_OP RS_OP
 %left	'+' '-'
 %left	'*' '/' '%'
-%right	

Re: [HACKERS] Logical Replication WIP

[Petr Jelinek]

On 20/01/17 17:33, Jaime Casanova wrote:
> On 20 January 2017 at 11:25, Petr Jelinek  
> wrote:
>> On 20/01/17 17:05, Fujii Masao wrote:
>>> On Fri, Jan 20, 2017 at 11:08 PM, Peter Eisentraut
>>>  wrote:
 On 1/19/17 5:01 PM, Petr Jelinek wrote:
> There were some conflicting changes committed today so I rebased the
> patch on top of them.
>
> Other than that nothing much has changed, I removed the separate sync
> commit patch, included the rename patch in the patchset and fixed the
> bug around pg_subscription catalog reported by Erik Rijkers.

 Committed.
>>>
>>> Sorry I've not followed the discussion about logical replication at all, but
>>> why does logical replication launcher need to start up by default?
>>>
>>
>> Because running subscriptions is allowed by default. You'd need to set
>> max_logical_replication_workers to 0 to disable that.
>>
> 
> surely wal_level < logical shouldn't start a logical replication
> launcher, and after an initdb wal_level is only replica
> 

Launcher is needed for subscriptions, subscriptions don't depend on
wal_level.

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, 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] Logical Replication WIP

[Jaime Casanova]

On 20 January 2017 at 11:25, Petr Jelinek  wrote:
> On 20/01/17 17:05, Fujii Masao wrote:
>> On Fri, Jan 20, 2017 at 11:08 PM, Peter Eisentraut
>>  wrote:
>>> On 1/19/17 5:01 PM, Petr Jelinek wrote:
 There were some conflicting changes committed today so I rebased the
 patch on top of them.

 Other than that nothing much has changed, I removed the separate sync
 commit patch, included the rename patch in the patchset and fixed the
 bug around pg_subscription catalog reported by Erik Rijkers.
>>>
>>> Committed.
>>
>> Sorry I've not followed the discussion about logical replication at all, but
>> why does logical replication launcher need to start up by default?
>>
>
> Because running subscriptions is allowed by default. You'd need to set
> max_logical_replication_workers to 0 to disable that.
>

surely wal_level < logical shouldn't start a logical replication
launcher, and after an initdb wal_level is only replica


-- 
Jaime Casanova  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] Logical Replication WIP

[Petr Jelinek]

On 20/01/17 17:05, Fujii Masao wrote:
> On Fri, Jan 20, 2017 at 11:08 PM, Peter Eisentraut
>  wrote:
>> On 1/19/17 5:01 PM, Petr Jelinek wrote:
>>> There were some conflicting changes committed today so I rebased the
>>> patch on top of them.
>>>
>>> Other than that nothing much has changed, I removed the separate sync
>>> commit patch, included the rename patch in the patchset and fixed the
>>> bug around pg_subscription catalog reported by Erik Rijkers.
>>
>> Committed.
> 
> Sorry I've not followed the discussion about logical replication at all, but
> why does logical replication launcher need to start up by default?
> 

Because running subscriptions is allowed by default. You'd need to set
max_logical_replication_workers to 0 to disable that.

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, 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] Logical Replication WIP

[Petr Jelinek]

On 20/01/17 15:08, Peter Eisentraut wrote:
> On 1/19/17 5:01 PM, Petr Jelinek wrote:
>> There were some conflicting changes committed today so I rebased the
>> patch on top of them.
>>
>> Other than that nothing much has changed, I removed the separate sync
>> commit patch, included the rename patch in the patchset and fixed the
>> bug around pg_subscription catalog reported by Erik Rijkers.
> 
> Committed.  I haven't reviewed the rename patch yet, so I'll get back to
> that later.
> 

Hi,

Thanks!

Here is fix for the dependency mess.

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services
diff --git a/src/backend/commands/publicationcmds.c b/src/backend/commands/publicationcmds.c
index 21e523d..9791f43 100644
--- a/src/backend/commands/publicationcmds.c
+++ b/src/backend/commands/publicationcmds.c
@@ -236,6 +236,8 @@ CreatePublication(CreatePublicationStmt *stmt)
 
 	heap_close(rel, RowExclusiveLock);
 
+	recordDependencyOnOwner(PublicationRelationId, puboid, GetUserId());
+
 	InvokeObjectPostCreateHook(PublicationRelationId, puboid, 0);
 
 	return myself;
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 1448ee3..00f2a5f 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -313,6 +313,8 @@ CreateSubscription(CreateSubscriptionStmt *stmt)
 
 	ObjectAddressSet(myself, SubscriptionRelationId, subid);
 
+	recordDependencyOnOwner(SubscriptionRelationId, subid, GetUserId());
+
 	InvokeObjectPostCreateHook(SubscriptionRelationId, subid, 0);
 
 	return myself;
@@ -493,6 +495,10 @@ DropSubscription(DropSubscriptionStmt *stmt)
 
 	ReleaseSysCache(tup);
 
+	/* Clean up the depenencies. */
+	deleteSharedDependencyRecordsFor(SubscriptionRelationId, subid,
+	 InvalidOid);
+
 	/* Protect against launcher restarting the worker. */
 	LWLockAcquire(LogicalRepLauncherLock, LW_EXCLUSIVE);
 

-- 
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] pgbench - allow backslash continuations in \set expressions

[Tom Lane]

Fabien COELHO  writes:
>> Probably the easiest fix is to add a rule that explicitly matches this 
>> situation:
>> {nonspace}+{continuation}  { ... throw back 2 chars and return the rest ... }

> Well, as the continuation characters must be ignored, so there is no need 
> to throw them back, just adding the special case is enough?

True, for the moment.  If we ever put an action into the continuation rule
(e.g. to count line numbers in the script) it might be better the other
way, but for now this is fine.

> Note anyway that it is not necessarily what people may intend when using a 
> continuation:

Yeah, but as you say this varies from one language to another.  I'm fine
with treating the continuation marker like whitespace.

Pushed with cosmetic adjustments (mostly, adding comments).

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] Fix documentation typo

[Fujii Masao]

On Fri, Jan 20, 2017 at 8:02 PM, Ishii Ayumi  wrote:
> Hi,
>
>> Here is a patch that adds temporary column's description to
>> pg_replication_slots document.
>
> Attached patch is updated version  to add detailed description.
>
> Regards,
> --
> Ayumi Ishii
>
> 2017-01-19 15:56 GMT+09:00 Ishii Ayumi :
>> Hi,
>>
>> Here is a patch that adds temporary column's description to
>> pg_replication_slots document.

This is the oversight of commit a924c32. Thanks for the patch! Pushed.

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] Logical Replication WIP

[Fujii Masao]

On Fri, Jan 20, 2017 at 11:08 PM, Peter Eisentraut
 wrote:
> On 1/19/17 5:01 PM, Petr Jelinek wrote:
>> There were some conflicting changes committed today so I rebased the
>> patch on top of them.
>>
>> Other than that nothing much has changed, I removed the separate sync
>> commit patch, included the rename patch in the patchset and fixed the
>> bug around pg_subscription catalog reported by Erik Rijkers.
>
> Committed.

Sorry I've not followed the discussion about logical replication at all, but
why does logical replication launcher need to start up by default?

$ initdb -D data
$ pg_ctl -D data start

When I ran the above commands, I got the following message and
found that the bgworker for logical replicatdion launcher was running.

LOG:  logical replication launcher started

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] Packages: Again

[Joshua D. Drake]

On 01/17/2017 09:26 AM, Robert Haas wrote:

On Fri, Jan 13, 2017 at 7:24 PM, Peter Geoghegan  wrote:

MERGE isn't UPSERT, and isn't even in competition with UPSERT as a
feature. I've written reams of text explaining why this is so in
precise detail, ...



Hello,

This is the webinar that started this whole thread (well the original 
thread, not this weird MERGE/UPSERT stuff):


https://www.commandprompt.com/blog/postgresql_for_oracle_people/

Thank you to everyone that responded. You will see in this Webinar that 
at least from the Oracle people perspective, PostgreSQL is not an option 
unless it has packages.


The other item that people bring up a few times is Oracle Forms but as 
that is actually external (although dependent) on Oracle, I don't see 
that as our responsibility.


Sincerely,

JD


--
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

[HACKERS] logical-replication.sgml improvements

[Erik Rijkers]

logical-replication.sgml.diff changes

Erik Rijkers--- doc/src/sgml/logical-replication.sgml.orig	2017-01-20 16:46:25.0 +0100
+++ doc/src/sgml/logical-replication.sgml	2017-01-20 16:46:55.0 +0100
@@ -24,7 +24,7 @@
  
 
  
-  Logical replication sends the changes on the publisher to the subscriber as
+  Logical replication sends changes on the publisher to the subscriber as
   they occur in real-time.  The subscriber applies the data in the same order
   as the publisher so that transactional consistency is guaranteed for
   publications within a single subscription.  This method of data replication
@@ -44,7 +44,7 @@
 

 
- Firing triggers for individual changes as they are incoming to
+ Firing triggers for individual changes as they arrive on the
  subscriber.
 

@@ -81,19 +81,19 @@
   instance and can be used as a publisher for other databases by defining its
   own publications.  When the subscriber is treated as read-only by
   application, there will be no conflicts from a single subscription.  On the
-  other hand, if there are other writes done either by application or other
-  subscribers to the same set of tables conflicts can arise.
+  other hand, if there are other writes done either by an application or by other
+  subscribers to the same set of tables, conflicts can arise.
  
 
  
   Publication
 
   
-   A publication object can be defined on any physical
+   A publication can be defined on any physical
replication master.  The node where a publication is defined is referred to
as publisher.  A publication is a set of changes
-   generated from a group of tables, and might also be described as a change
-   set or replication set.  Each publication exists in only one database.
+   generated from a table or a group of tables, and might also be described as
+   a change set or replication set.  Each publication exists in only one database.
   
 
   
@@ -105,9 +105,9 @@
   
 
   
-   Publications can choose to limit the changes they produce to show
+   Publications can choose to limit the changes they produce to
any combination of INSERT, UPDATE, and
-   DELETE in a similar way to the way triggers are fired by
+   DELETE, similar to how triggers are fired by
particular event types.  If a table without a REPLICA
IDENTITY is added to a publication that
replicates UPDATE or DELETE
@@ -121,7 +121,7 @@
 
   
A publication is created using the 
-   command and may be later altered or dropped using corresponding commands.
+   command and may later be altered or dropped using corresponding commands.
   
 
   
@@ -141,7 +141,7 @@
replication.  The node where a subscription is defined is referred to as
the subscriber.  Subscription defines the connection
to another database and set of publications (one or more) to which it wants
-   to be subscribed.
+   to subscribe.
   
 
   
@@ -153,7 +153,7 @@
   
A subscriber node may have multiple subscriptions if desired.  It is
possible to define multiple subscriptions between a single
-   publisher-subscriber pair, in which case extra care must be taken to ensure
+   publisher-subscriber pair, in which case care must be taken to ensure
that the subscribed publication objects don't overlap.
   
 
@@ -164,8 +164,8 @@
 
   
Subscriptions are not dumped by pg_dump by default but
-   can be requested using the command-line
-   option --subscriptions.
+   this can be requested using the command-line
+   option --include-subscriptions.
   
 
   
@@ -183,7 +183,7 @@
 
   
The schema definitions are not replicated and the published tables must
-   exist on the subsriber for replication to work.  Only regular tables may be
+   exist on the subscriber.  Only regular tables may be
the target of replication.  For example, you can't replicate to a view.
   
 
@@ -203,9 +203,9 @@
   Conflicts
 
   
-   The logical replication behaves similarly to normal DML operations in that
+   Logical replication behaves similarly to normal DML operations in that
the data will be updated even if it was changed locally on the subscriber
-   node.  If the incoming data violates any constraints the replication will
+   node.  If incoming data violates any constraints the replication will
stop.  This is referred to as a conflict.  When
replicating UPDATE or DELETE
operations, missing data will not produce a conflict and such operations
@@ -224,8 +224,8 @@
transaction that conflicts with the existing data.  The transaction can be
skipped by calling the 
pg_replication_origin_advance() function with
-   a node_name corresponding to the subscription name.
-   The current position of origins can be seen in the
+   a node_name corresponding to the subscription name,
+   and a position.  The current position of origins can be seen in the

pg_replication_origin_status system view.
   
@@ -246,8 +246,8 @@
   
Logical replication is built with an architecture 

Re: [HACKERS] Error building docs

[Tom Lane]

Andreas Joseph Krogh  writes:
> I'm getting this error building docs (from commit 
> e4c27f5befbfc80a1bf96fc93256dce08b148238):

Did it work for you before?

>  osx -D. -x lower -i include-xslt-index postgres.sgml >postgres.xmltmp
>  osx:postgres.sgml:3:55:W: cannot generate system identifier for public text 
> "-//OASIS//DTD DocBook V4.2//EN"
>  osx:postgres.sgml:12:0:E: reference to entity "BOOK" for which no system 
> identifier could be generated

This looks like you don't have a complete installation of the docbook
scripts.

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] Error building docs

[Andreas Joseph Krogh]

Hi,
 
I'm getting this error building docs (from commit 
e4c27f5befbfc80a1bf96fc93256dce08b148238):
 
$ make docs  
 make -C doc all
 make[1]: Entering directory '/home/andreak/dev/postgresql/doc'
 make -C src all
 make[2]: Entering directory '/home/andreak/dev/postgresql/doc/src'
 make -C sgml all
 make[3]: Entering directory '/home/andreak/dev/postgresql/doc/src/sgml'
 { \
  echo ""; \
  echo ""; \
 } > version.sgml
 '/usr/bin/perl' ./mk_feature_tables.pl YES 
../../../src/backend/catalog/sql_feature_packages.txt 
../../../src/backend/catalog/sql_features.txt > features-supported.sgml
 '/usr/bin/perl' ./mk_feature_tables.pl NO 
../../../src/backend/catalog/sql_feature_packages.txt 
../../../src/backend/catalog/sql_features.txt > features-unsupported.sgml
 '/usr/bin/perl' ./generate-errcodes-table.pl 
../../../src/backend/utils/errcodes.txt > errcodes-table.sgml
 osx -D. -x lower -i include-xslt-index postgres.sgml >postgres.xmltmp
 osx:postgres.sgml:3:55:W: cannot generate system identifier for public text 
"-//OASIS//DTD DocBook V4.2//EN"
 osx:postgres.sgml:12:0:E: reference to entity "BOOK" for which no system 
identifier could be generated
 osx:postgres.sgml:3:0: entity was defined here
 osx:postgres.sgml:12:0:E: DTD did not contain element declaration for 
document type name
 osx:postgres.sgml:14:9:E: there is no attribute "ID"
 osx:postgres.sgml:14:19:E: element "BOOK" undefined
 osx:postgres.sgml:15:7:E: element "TITLE" undefined
 osx:postgres.sgml:17:10:E: element "BOOKINFO" undefined
 osx:postgres.sgml:18:13:E: element "CORPAUTHOR" undefined
 osx:postgres.sgml:19:14:E: element "PRODUCTNAME" undefined
 osx:postgres.sgml:20:16:E: element "PRODUCTNUMBER" undefined

  
$ osx --version 
 osx:I: "OpenSP" version "1.5.2"

 Any hints?
 
Thanks.
 
-- Andreas Joseph Krogh
CTO / Partner - Visena AS
Mobile: +47 909 56 963
andr...@visena.com 
www.visena.com 

Re: [HACKERS] Parallel Index Scans

[Amit Kapila]

On Fri, Jan 20, 2017 at 3:41 AM, Robert Haas  wrote:
> On Wed, Jan 18, 2017 at 8:03 AM, Amit Kapila  wrote:
>>> Why do we need separate AM support for index_parallelrescan()?  Why
>>> can't index_rescan() cover that case?
>>
>> The reason is that sometime index_rescan is called when we have to
>> just update runtime scankeys in index and we don't want to reset
>> parallel scan for that.
>
> Why not?  I see the code, but the comments don't seem to offer any
> justification for that.  And it seems wrong to me.  If the scan keys
> change, surely that only makes sense if we restart the scan.  You
> can't just blindly continue the same scan if the keys have changed,
> can you?
>

Sure, if scan keys have changed then we can't continue, but this is
the case where runtime keys are first time initialized.

if (node->iss_NumRuntimeKeys != 0 && !node->iss_RuntimeKeysReady)

In the above check, the second part of the check
(!node->iss_RuntimeKeysReady) ensures that it is for the first time.
Now, let me give you an example to explain what bad can happen if we
allow resetting parallel scan in this case.  Consider a query like
select * from t1 where c1 < parallel_index(10);, in this if we allow
resetting parallel scan descriptor during first time initialization of
runtime keys, it can easily corrupt the parallel scan state.  Suppose
leader has taken the lead and is scanning some page and worker reaches
to initialize its keys in ExecReScanIndexScan(), if worker resets the
parallel scan, then it will corrupt the state of the parallel scan
state.

If you agree with the above explanation, then I will expand the
comments in next update.


Just in case you want to debug the above query, below is the schema
and necessary steps.

create or replace function parallel_index(a integer) returns integer
as $$
begin
return a + 1;
end;
$$ language plpgsql STABLE PARALLEL SAFE;


create table t1(c1 int, c2 char(20));
insert into t1 values(generate_series(1,30),'aaa');
create index idx_t1 on t1 (c1);

set max_parallel_workers_per_gather=1;
set parallel_setup_cost=0;
set parallel_tuple_cost=0;
set min_parallel_relation_size=0;
set enable_bitmapscan=off;
set enable_seqscan=off;

select * from t1 where c1 < parallel_index(1000);

> I think the reason that this isn't breaking for you is that it's
> difficult or impossible to get a parallel index scan someplace where
> the keys would change at runtime.  Normally, the parallel scan is on
> the driving relation, and so there are no runtime keys.  We currently
> have no way for a parallel scan to occur on the inner side of a nested
> loop unless there's an intervening Gather node - and in that case the
> keys can't change without relaunching the workers.  It's hard to see
> how it could work otherwise.  For example, suppose you had something
> like this:
>
> Gather
> -> Nested Loop
>   -> Parallel Seq Scan on a
>   -> Hash Join
> -> Seq Scan on b
> -> Parallel Shared Hash
>   -> Parallel Index Scan on c
>   Index Cond: c.x = a.x
>
> Well, the problem here is that there's nothing to ensure that various
> workers who are cooperating to build the hash table all have the same
> value for a.x, nor is there any thing to ensure that they'll all get
> done with the shared hash table at the same time.  So this is just
> chaos.  I think we have to disallow this kind of plan as nonsensical.
> Given that, I'm not sure a reset of the runtime keys can ever really
> happen.  Have you investigated this?
>

Having parallelism on the right side under gather can only be possible
after Parallel hash patch of Robert, so maybe some investigation is
needed when we review that patch.  Let me know if you want me to
investigate something more after my explanation above.

> I extracted the generic portions of this infrastructure (i.e. not the
> btree-specific stuff) and spent some time working on it today.  The
> big thing I fixed was the documentation, which you added in a fairly
> illogical part of the file.
>

Hmm, it is not illogical. All the functions are described in the same
order as they are declared in IndexAmRoutine structure and I have
followed the same. I think both amestimateparallelscan and
aminitparallelscan should be added one para down which says (The
purpose of an index .. The scan-related functions that an index access
method must or may provide are:).

>  You had all of the new functions for
> supporting parallel scans in a section that explicitly says it's for
> mandatory functions, and which precedes the section on regular
> non-parallel scans.
>

I think that section should say "must or may provide" instead of "must
provide" as the functions amcanreturn and amproperty are optional and
are described in that section.

>  I moved the documentation for all three new
> methods to the same place, added some explanation of parallel scans in
> general, and rewrote the descriptions for the individual functions to
> be more clear.

Re: [HACKERS] Logical Replication WIP

[Peter Eisentraut]

On 1/19/17 5:01 PM, Petr Jelinek wrote:
> There were some conflicting changes committed today so I rebased the
> patch on top of them.
> 
> Other than that nothing much has changed, I removed the separate sync
> commit patch, included the rename patch in the patchset and fixed the
> bug around pg_subscription catalog reported by Erik Rijkers.

Committed.  I haven't reviewed the rename patch yet, so I'll get back to
that later.

-- 
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] Improvements in psql hooks for variables

[Tom Lane]

"Daniel Verite"  writes:
> Setting ENCODING has no effect, like DBNAME, USER, HOST and PORT.
> In a way, it's a read-only variable that's here to inform the user,
> not as a means to change the encoding (\encoding does that and
> has proper support for tab completion)

Right.

> What we could do as of this patch is emit an error when we try
> to change ENCODING, with a hook returning false and
> a proper error message hinting to \encoding.

I think that the current behavior is intentional: it avoids making
those variables reserved.  That is, if you're unaware that psql
sets them and try to use them for your own purposes, it will work.

However, it only really works if psql never overwrites the values
after startup, whereas I believe all of these are overwritten by
a \c command.

So maybe it's more user-friendly to make these variables fully
reserved, even at the risk of breaking existing scripts.  But
I don't think it's exactly an open-and-shut question.

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] [JDBC] SEGFAULT in HEAD with replication

[Jorge Solórzano]

Thanks tom, I confirm that ba61a04

fixes
the issue. Now CI is passing.

Jorge Solórzano
me.jorsol.com

On Thu, Jan 19, 2017 at 6:09 PM, Tom Lane  wrote:

> I wrote:
> > Hmm ... that line was last touched by ab1f0c822, so I'm betting that
> > I broke it somehow, but I'm not sure how.
> > It looks like S_3 might have been parsed from a totally empty source
> > string?  But if that's the trigger, I'd think it'd be entirely trivial
> > to reproduce.
>
> Oh, duh: the reason it's not trivial to reproduce is you have to try
> to bind an empty prepared statement *in an already-aborted transaction*.
>
> Will push a fix in a bit.
>
> regards, tom lane
>

Re: [HACKERS] Improvements in psql hooks for variables

[Daniel Verite]

Ashutosh Sharma wrote:

> postgres=# \echo :ENCODING
> UTF8
> postgres=# \set ENCODING xyz
> postgres=# \echo :ENCODING
> xyz
> 
> I think currently we are not even showing what are the different valid
> encoding names to the end users like we show it for other built-in
> variables
> VERBOSITY, ECHO etc. I mean if i run '\set VERBOSITY' followed by tab
> command it does show me the valid values for VERBOSITY but not for
> ENCODING.

Setting ENCODING has no effect, like DBNAME, USER, HOST and PORT.
In a way, it's a read-only variable that's here to inform the user,
not as a means to change the encoding (\encoding does that and
has proper support for tab completion)

What we could do as of this patch is emit an error when we try
to change ENCODING, with a hook returning false and
a proper error message hinting to \encoding.

I'm working on adding such messages to other variables.

Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


-- 
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] Fix minor race in commit_ts SLRU truncation vs lookups

[Alvaro Herrera]

Peter Eisentraut wrote:
> On 1/19/17 10:06 AM, Alvaro Herrera wrote:
> >> Also, I wonder whether we should not in vacuum.c change the order of the
> >> calls of SetTransactionIdLimit() and SetMultiXactIdLimit() as well, just
> >> to keep everything consistent.
> > 
> > I am wary of doing that.  The current coding is well battle-tested by
> > now, but doing things in the opposite order, not at all.  Pending some
> > testing to show that there is no problem with a change, I would leave
> > things as they are.
> 
> I appreciate this hesitation.
> 
> The issue the patch under discussion is addressing is that we have a
> user-space interface to read information about commit timestamps.  So if
> we truncate away old information before we update the mark where the
> good information starts, then we get the race.

Correct.

> We don't have a user-space interface reading, say, the clog, but it
> doesn't seem implausible for that to exist at some point.  How would
> this code have to be structured then?

One option would be to add another limit Xid which advances before the
truncation but which is not used for other decisions other than limiting
what can users consult.  Another option is not to implement direct reads
from the clog.  Yet another option is that before we add such interface
somebody produces proof that the problem does not, in fact, exist.  I
think producing proof is onerous enough that nobody will step up to it
until there is some real need for user-invoked clog lookups.  (We've
gone so long without them, perhaps we will never need them.)

> > What I fear is: what
> > happens if a concurrent checkpoint reads the values between the two
> > operations, and a crash occurs?  I think that the checkpoint might save
> > the updated values, so after crash recovery the truncate would not be
> > executed, possibly leaving files around.  Leaving files around might be
> > dangerous for multixacts at least (it's probably harmless for xids).
> 
> Why is leaving files around dangerous?

Leftover files could confuse the truncation algorithm.  We already had
this problem for multixacts, and it took a lot of sweat and blood to get
it fixed.  The algorithm has since been fixed, but the mechanism is
delicate enough that I am afraid that tinkering with it may break it.

> If this is a problem, why is it not a problem for commit timestamps?
> I don't understand why these different SLRU uses are different.

commit_ts heavily depends on the clog cycle, so my thinking is that if
clog is protected enough, then so is commit_ts.  Given a strong clog, I
expect commit_ts not to break.  If I make clog brittle, will that break
commit_ts too?  Perhaps.  Maybe not, but again that would require more
study.

I have looked at clog a bit more this morning and I think perhaps it is
safe to make it work in the same way as commit_ts -- but somebody would
have to go to the trouble of verifying that it is.

> Yeah, we could go ahead with this patch as is and it might be fine, but
> I feel like we don't understand the broader issue completely yet.

ISTM the more complicated uses of slru.c have turned out to have a lot
of emergent properties that weren't completely understood, so yeah I
agree that we don't (or at least I don't, and most people don't either).
Someday, somebody will rewrite slru.c and/or remove multixacts, and
these problems will all go away.

-- 
Álvaro Herrerahttps://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 to implement pg_current_logfile() function

[Michael Paquier]

On Fri, Jan 20, 2017 at 10:11 PM, Alvaro Herrera
 wrote:
>> diff --git a/src/backend/replication/basebackup.c 
>> b/src/backend/replication/basebackup.c
>
>> @@ -148,6 +149,9 @@ static const char *excludeFiles[] =
>>   /* Skip auto conf temporary file. */
>>   PG_AUTOCONF_FILENAME ".tmp",
>>
>> + /* Skip current log file temporary file */
>> + LOG_METAINFO_DATAFILE_TMP,
>> +
>
> Sorry if this has already been answered, but why are we not also
> skipping LOG_METAINFO_DATAFILE itself?  I can't see a scenario where
> it's useful to have that file in a base backup, since it's very likely
> that the log file used when the backup is restored will be different.

I have done the same remark upthread, and Gilles has pointed out that
it would be useful to get the last log file that was used by a backup.
Including this file represents no harm as well.
-- 
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] Patch to implement pg_current_logfile() function

[Alvaro Herrera]

> diff --git a/src/backend/replication/basebackup.c 
> b/src/backend/replication/basebackup.c

> @@ -148,6 +149,9 @@ static const char *excludeFiles[] =
>   /* Skip auto conf temporary file. */
>   PG_AUTOCONF_FILENAME ".tmp",
>  
> + /* Skip current log file temporary file */
> + LOG_METAINFO_DATAFILE_TMP,
> +

Sorry if this has already been answered, but why are we not also
skipping LOG_METAINFO_DATAFILE itself?  I can't see a scenario where
it's useful to have that file in a base backup, since it's very likely
that the log file used when the backup is restored will be different.

-- 
Álvaro Herrerahttps://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

[HACKERS] Valgrind-detected bug in partitioning code

[Tom Lane]

skink has been unhappy since commit d26fa4f went in, but I think
that just exposed a pre-existing bug.  Running valgrind here
duplicates the failure:

==00:00:02:01.653 16626== Conditional jump or move depends on uninitialised 
value(s)
==00:00:02:01.653 16626==at 0x4BDF6B: btint4cmp (nbtcompare.c:97)
==00:00:02:01.653 16626==by 0x81D6BE: FunctionCall2Coll (fmgr.c:1318)
==00:00:02:01.653 16626==by 0x52D584: partition_bounds_equal 
(partition.c:627)
==00:00:02:01.653 16626==by 0x80CF8E: RelationClearRelation 
(relcache.c:1203)
==00:00:02:01.653 16626==by 0x80E601: RelationCacheInvalidateEntry 
(relcache.c:2662)
==00:00:02:01.653 16626==by 0x803DD6: LocalExecuteInvalidationMessage 
(inval.c:568)
==00:00:02:01.653 16626==by 0x803F53: ProcessInvalidationMessages.clone.0 
(inval.c:444)
==00:00:02:01.653 16626==by 0x8040C8: CommandEndInvalidationMessages 
(inval.c:1056)
==00:00:02:01.653 16626==by 0x80C719: RelationSetNewRelfilenode 
(relcache.c:3490)
==00:00:02:01.653 16626==by 0x5CD50A: ExecuteTruncate (tablecmds.c:1393)
==00:00:02:01.653 16626==by 0x721AC7: standard_ProcessUtility 
(utility.c:532)
==00:00:02:01.653 16626==by 0x71D943: PortalRunUtility (pquery.c:1163)

IOW, partition_bounds_equal() is testing uninitialized memory during
a TRUNCATE on a partitioned table.

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] pgbench - allow backslash continuations in \set expressions

[Fabien COELHO]

You have rules for

{nonspace}+ [...]
{continuation}

Remember that flex always takes the rule that produces the longest match
starting at the current point.  {space}+ and {newline} don't conflict with
continuations, but {nonspace}+ does:


Indeed, I totally overlooked "{nonspace}+".


[...] I think this is surprising and inconsistent.


Sure.

Probably the easiest fix is to add a rule that explicitly matches this 
situation:


{nonspace}+{continuation}  { ... throw back 2 chars and return the rest ... }


Well, as the continuation characters must be ignored, so there is no need 
to throw them back, just adding the special case is enough?


Attached a patch which adds the rule and just sends the found word, plus a 
test script which also exercises this particular case.


Note anyway that it is not necessarily what people may intend when using a 
continuation:


  foo\
bla

might mean "foobla" rather than "foo" then "bla". For instance with bash:

 sh>ec\
 > ho 1
 1

But the same trick in python gives a syntax error:

  py> print\
  ... (1)
  1 # ok...
  py> pri\
  ... nt(1)
File "", line 2
  nt(1)
   ^
  SyntaxError: invalid syntax

I think it fine if pgbench behaves as python.

--
Fabien.diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml
index 3fb29f8..eb0e8ac 100644
--- a/doc/src/sgml/ref/pgbench.sgml
+++ b/doc/src/sgml/ref/pgbench.sgml
@@ -810,6 +810,7 @@ pgbench  options  dbname
   
Script file meta commands begin with a backslash (\) and
extend to the end of the line.
+   They can spread over several lines with backslash-return continuations.
Arguments to a meta command are separated by white space.
These meta commands are supported:
   
@@ -838,7 +839,8 @@ pgbench  options  dbname
   Examples:
 
 \set ntellers 10 * :scale
-\set aid (1021 * random(1, 10 * :scale)) % (10 * :scale) + 1
+\set aid (1021 * random(1, 10 * :scale)) % \
+   (10 * :scale) + 1
 
 

diff --git a/src/bin/pgbench/exprscan.l b/src/bin/pgbench/exprscan.l
index 9a3be3d..ec58ae3 100644
--- a/src/bin/pgbench/exprscan.l
+++ b/src/bin/pgbench/exprscan.l
@@ -65,6 +65,7 @@ alnum			[a-zA-Z0-9_]
 space			[ \t\r\f\v]
 nonspace		[^ \t\r\f\v\n]
 newline			[\n]
+continuation	\\{newline}
 
 /* Exclusive states */
 %x EXPR
@@ -90,6 +91,11 @@ newline			[\n]
 
 	/* INITIAL state */
 
+{nonspace}+{continuation}		{
+	/* Found "word\\\n", just emit word and return it */
+	psqlscan_emit(cur_state, yytext, yyleng-2);
+	return 1;
+}
 {nonspace}+		{
 	/* Found a word, emit and return it */
 	psqlscan_emit(cur_state, yytext, yyleng);
@@ -104,6 +110,8 @@ newline			[\n]
 	return 0;
 }
 
+{continuation}	{ /* ignore */ }
+
 	/* EXPR state */
 
 {
@@ -138,6 +146,8 @@ newline			[\n]
 	return FUNCTION;
 }
 
+{continuation}	{ /* ignore */ }
+
 {newline}		{
 	/* report end of command */
 	last_was_newline = true;


cont.sql
Description: application/sql

-- 
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] Improvements in psql hooks for variables

[Ashutosh Sharma]

Hi,

I had a quick look into this patch and it seems to me like it takes
care of all the built-in variables except ENCODING type. I think we
need to apply such rule for ENCODING variable too.

postgres=# \echo :ENCODING
UTF8
postgres=# \set ENCODING xyz
postgres=# \echo :ENCODING
xyz

I think currently we are not even showing what are the different valid
encoding names to the end users like we show it for other built-in
variables
VERBOSITY, ECHO etc. I mean if i run '\set VERBOSITY' followed by tab
command it does show me the valid values for VERBOSITY but not for
ENCODING.

postgres=# \set VERBOSITY
default  terseverbose

Moreover, I feel we are just passing the error message to end users
for any bogus assignments but not the hint message showing the correct
set of values that are accepted.

postgres=# \set ECHO on
unrecognized value "on" for "ECHO"
\set: error while setting variable

Above error message should also have some expected values with it.

Please note that I haven't gone through the entire mail chain so not
sure if above thoughts have already been raised by others. Sorry about
that.

With Regards,
Ashutosh Sharma
EnterpriseDB: http://www.enterprisedb.com

On Mon, Jan 16, 2017 at 10:12 PM, Tom Lane  wrote:
> "Daniel Verite"  writes:
>>   Tom Lane wrote:
>>> Also, why aren't you using ParseVariableBool's existing ability to report
>>> errors?
>
>> Its' because there are two cases:
>> - either only a boolean can be given to the command or variable,
>> in which we let ParseVariableBool() tell:
>>unrecognized value "bogus" for "command": boolean expected
>> - or other parameters besides boolean are acceptable, in which case we
>> can't say "boolean expected", because in fact a boolean is no more or
>> less expected than the other valid values.
>
> Ah.  Maybe it's time for a ParseVariableEnum, or some other way of
> dealing with those cases in a more unified fashion.
>
>> Do we care about the "boolean expected" part of the error message?
>
> I'm not especially in love with that particular wording, but I'm doubtful
> that we should give up all indication of what valid values are, especially
> in the cases where there are more than just bool-equivalent values.
> I think the right thing to do here is to fix it so that the input routine
> has full information about all the valid values.  On the backend side,
> we've gone to considerable lengths to make sure that error messages are
> helpful for such cases, eg:
>
> regression=# set backslash_quote to foo;
> ERROR:  invalid value for parameter "backslash_quote": "foo"
> HINT:  Available values: safe_encoding, on, off.
>
> and I think it may be worth working equally hard here.
>
>> I get the idea, except that in this example, the compiler is
>> unhappy because popt->topt.expanded is not a bool, and an
>> explicit cast here would be kludgy.
>
> Yeah, you'll need an intermediate variable if you're trying to use
> ParseVariableBool for such a case.
>
> 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


-- 
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 ] pg_dump: new --custom-fetch-table and --custom-fetch-value parameters

[Andrea Urbani]

>> I have solve it adding two new parameters, --custom-fetch-table and
>> --custom-fetch-value, to fetch less records for the specified table(s).

> Giving the user the ability to change the fetch size sounds interesting,
> though do we really need to specify it per table? What about just a
> --fetch-size=X option?

...

> I don't particularly like the use of 'custom' in the name of the option,
> seems like it's just a noise word and not really necessary.

I have used "custom" parameters because I want to decrease the fetch size only 
on the tables with big bloab fields. If we remove the "custom-fetch-table" 
parameter and we provide only the "fetch-size" parameter all the tables will 
use the new fetch size and the execution time will be slower (according to my 
few tests). But just "fetch-size" will be faster to use and maybe more clear.
Well, how to go on? I add it to the commitfest and somebody will decide and fix 
it?
Please, let me know
Thank you
Andrea
 

Sent: Wednesday, December 21, 2016 at 6:44 PM
From: "Stephen Frost" 
To: "Andrea Urbani" 
Cc: pgsql-hackers@postgresql.org
Subject: Re: [HACKERS] [ patch ] pg_dump: new --custom-fetch-table and 
--custom-fetch-value parameters
Andrea,

* Andrea Urbani (matfan...@mail.com) wrote:
> I had a problem with a Postgresql 9.3.5 on 32 bit linux, old 2.6.26
> kernel:

Ok, though, to be clear, this is a feature request, so we wouldn't
back-patch adding this to pg_dump.

> I have solve it adding two new parameters, --custom-fetch-table and
> --custom-fetch-value, to fetch less records for the specified table(s).

Giving the user the ability to change the fetch size sounds interesting,
though do we really need to specify it per table? What about just a
--fetch-size=X option?

> This does not completely solve the problem, but it helps you to get more
> chance to be able to dump your database.

That is certainly a worthwhile goal.

>     pg_dump --dbname=healthorganizer --username=hor --column-inserts
> --custom-fetch-table='"tDocumentsFiles"' --custom-fetch-value=25

I don't particularly like the use of 'custom' in the name of the option,
seems like it's just a noise word and not really necessary.

> I haven't tested the documentation: too many problems while building it
> (also the original version, without my changes; probably I have bogus
> tools... and too less time to check/try...).
> Attached the patches for the master and REL9_6_STABLE.

I agree the documentation can be a bit of a pain, but there's a lot of
issues with the patch itself when it comes to the project style. The
indentation doesn't look like it's all correct, and multi-line comments
should be of the form:

/*
* text here
*/

Lastly, it'd be good to have this patch added to
https://commitfest.postgresql.org to have it formally reviewed in the
commitfest cycle coming up in January.

Thanks!

Stephen


-- 
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] Fix documentation typo

[Ishii Ayumi]

Hi,

> Here is a patch that adds temporary column's description to
> pg_replication_slots document.

Attached patch is updated version  to add detailed description.

Regards,
--
Ayumi Ishii

2017-01-19 15:56 GMT+09:00 Ishii Ayumi :
> Hi,
>
> Here is a patch that adds temporary column's description to
> pg_replication_slots document.
>
> Regards,
> --
> Ayumi Ishii
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 4930506..417274e 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -9007,6 +9007,13 @@ SELECT * FROM pg_locks pl LEFT JOIN pg_prepared_xacts ppx
  
 
  
+  temporary
+  boolean
+  
+  True if this is temporary replication slot. Temporary slots are not saved to disk and are automatically dropped on error or when the session has finished.
+ 
+
+ 
   active
   boolean
   

-- 
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] Parallel Index Scans

[Amit Kapila]

On Fri, Jan 20, 2017 at 12:59 AM, Robert Haas  wrote:
> On Thu, Jan 19, 2017 at 4:26 AM, Amit Kapila  wrote:
>> Fixed.
>
>
> If all of that were no issue, the considerations in
> TargetListSupportsBackwardScan could be a problem, too.  But I think
> there shouldn't be any issue having Gather just continue to return
> false.
>

You are right.  I have added that code under the assumption that if
the underlying node (in this case index scan) can support backward
scan, gather can also support.  I forgot/missed that
ExecSupportsBackwardScan is to support cursors operations.  Will fix
in next version of patch.


-- 
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] Vacuum: allow usage of more than 1GB of work mem

[Masahiko Sawada]

On Thu, Jan 19, 2017 at 8:31 PM, Claudio Freire  wrote:
> On Thu, Jan 19, 2017 at 6:33 AM, Anastasia Lubennikova
>  wrote:
>> 28.12.2016 23:43, Claudio Freire:
>>
>> Attached v4 patches with the requested fixes.
>>
>>
>> Sorry for being late, but the tests took a lot of time.
>
> I know. Takes me several days to run my test scripts once.
>
>> create table t1 as select i, md5(random()::text) from
>> generate_series(0,4) as i;
>> create index md5_idx ON  t1(md5);
>> update t1 set md5 = md5((random() * (100 + 500))::text);
>> vacuum;
>>
>> Patched vacuum used 2.9Gb of memory and vacuumed the index in one pass,
>> while for old version it took three passes (1GB+1GB+0.9GB).
>> Vacuum duration results:
>>
>> vanilla:
>> LOG: duration: 4359006.327 ms  statement: vacuum verbose t1;
>> patched:
>> LOG: duration: 3076827.378 ms  statement: vacuum verbose t1;
>>
>> We can see 30% vacuum speedup. I should note that this case can be
>> considered
>> as favorable to vanilla vacuum: the table is not that big, it has just one
>> index
>> and disk used is a fast fusionIO. We can expect even more gain on slower
>> disks.
>>
>> Thank you again for the patch. Hope to see it in 10.0.
>
> Cool. Thanks for the review and the tests.
>

I encountered a bug with following scenario.
1. Create table and disable autovacuum on that table.
2. Make about 20 dead tuples on the table.
3. SET maintenance_work_mem TO 1024
4. VACUUM

@@ -729,7 +759,7 @@ lazy_scan_heap(Relation onerel, int options,
LVRelStats *vacrelstats,
 * not to reset latestRemovedXid since we want
that value to be
 * valid.
 */
-   vacrelstats->num_dead_tuples = 0;
+   lazy_clear_dead_tuples(vacrelstats);
vacrelstats->num_index_scans++;

/* Report that we are once again scanning the heap */

I think that we should do vacrelstats->dead_tuples.num_entries = 0 as
well in lazy_clear_dead_tuples(). Once the amount of dead tuples
reached to maintenance_work_mem, lazy_scan_heap can never finish.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center


-- 
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] Microvacuum support for Hash Index

[Ashutosh Sharma]

Hi Jesper,

> I'm not seeing this deadlock with just the WAL v8 patch applied.
>

okay, Thanks for confirming that.

I would like to update you that I am not able to reproduce this issue
at my end. I suspect that the steps i am following might be slightly
different than your's. Could you please have a look at steps mentioned
below and confirm if there is something different that I am doing.

Firstly, I am running the test-case on following git commit in head:


commit ba61a04bc7fefeee03416d9911eb825c4897c223
Author: Tom Lane 
Date:   Thu Jan 19 19:52:13 2017 -0500

Avoid core dump for empty prepared statement in an aborted transaction.

Brown-paper-bag bug in commit ab1f0c822: the old code here coped with
null CachedPlanSource.raw_parse_tree, the new code not so much.
Per report from Dave Cramer.


On top of above commit, I have applied WAL v8 patch for hash index and
MV v5 patch.

Now, with an --enable-cassert build I am following below steps:

1) Created a 'test' database

2) psql -d test -f ~/ddl.sql

where ddl.sql is,

-- ddl.sql --
CREATE TABLE test AS SELECT generate_series(1, 10) AS id, 0 AS val;
CREATE INDEX IF NOT EXISTS idx_id ON test USING hash (id);
CREATE INDEX IF NOT EXISTS idx_val ON test USING hash (val);
ANALYZE;
-- ddl.sql --

3) pgbench -M prepared -c 10 -j 10 -T 1800 -f ~/test.sql test

where test.sql is,

-- test.sql --
\set id random(1,10)
\set val random(0,10)
BEGIN;
UPDATE test SET val = :val WHERE id = :id;
COMMIT;
-- test.sql --


Machine details are as follows:

Architecture:  x86_64
CPU op-mode(s):32-bit, 64-bit
Byte Order:Little Endian
CPU(s):128
On-line CPU(s) list:   0-127
Thread(s) per core:2
Core(s) per socket:8
Socket(s): 8

Also, It would be great if you could confirm as if you have been
getting this issue repeatedly. Thanks.

With Regards,
Ashutosh Sharma
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: [WIP] RE: [HACKERS] DECLARE STATEMENT setting up a connection in ECPG

[Ideriha, Takeshi]

> Idehira-san, this is a very intrusive patch... It really needs a specific
> reviewer to begin with, but really it would be nice if you could review 
> someone
> else's patch, with a difficulty rather equivalent to what we have here.

Michael, thank you for taking a look at my patch and giving me an advice.
And sorry that I didn't follow a procedure of developing postgresql.
I think I should have sent a system design idea or small patch first and made 
it bigger and bigger step by step
instead of dumping a huge patch suddenly.

Yeah, I'm going to reviewing hackers' patches.

> By the way, I have been able to crash your patch when running the regression
> tests:

Thank you for checking and I'm going to handle this.

Regards, 
Ideriha Takeshi

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

[HACKERS] Fix a comment in feelist.c

[Yugo Nagata]

Hi,

I've found a mistake in a comment of StrategyNotifyBgWriter
in freelist.c. bgwriterLatch was replaced by bgwprocno in
the following commit, but this is remained in the comment.

commit d72731a70450b5e7084991b9caa15cb58a2820df
Author: Andres Freund 
Date:   Thu Dec 25 18:24:20 2014 +0100

Lockless StrategyGetBuffer clock sweep hot path.

Attached a patch.

-- 
Yugo Nagata 
diff --git a/src/backend/storage/buffer/freelist.c b/src/backend/storage/buffer/freelist.c
index b68ab20..5d0a636 100644
--- a/src/backend/storage/buffer/freelist.c
+++ b/src/backend/storage/buffer/freelist.c
@@ -406,8 +406,8 @@ StrategySyncStart(uint32 *complete_passes, uint32 *num_buf_alloc)
 /*
  * StrategyNotifyBgWriter -- set or clear allocation notification latch
  *
- * If bgwriterLatch isn't NULL, the next invocation of StrategyGetBuffer will
- * set that latch.  Pass NULL to clear the pending notification before it
+ * If bgwprocno isn't -1, the next invocation of StrategyGetBuffer will
+ * set that latch.  Pass -1 to clear the pending notification before it
  * happens.  This feature is used by the bgwriter process to wake itself up
  * from hibernation, and is not meant for anybody else to use.
  */

-- 
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] Password identifiers, protocol aging and SCRAM protocol

[Michael Paquier]

On Thu, Jan 19, 2017 at 6:17 PM, Simon Riggs  wrote:
> We seem to be caught between adding lots of new things as parameters
> and adding new detail into pg_hba.conf.
>
> Parameters like password_encryption are difficult here because they
> essentially repeat what has already been said in the pg_hba.conf. If
> we have two entries in pg_hba.conf, one saying md5 and the other
> saying "scram" (or whatever), what would we set password_encryption
> to? It seems clear to me that if the pg_hba.conf says md5 then
> password_encryption should be md5 and if pg_hba.conf says scram then
> it should be scram.
>
> I'd like to float another idea, as a way of finding a way forwards
> that will last over time
>
> * pg_hba.conf entry would say sasl='methodX' (no spaces)
> * we have a new catalog called pg_sasl that allows us to add new
> methods, with appropriate function calls

This would make sense if we support a mountain of protocols and that
we want to have a handler with a set of APIs used for authentication.
This is a grade higher than simple SCRAM, and this basically requires
to design a set of generic routines that are fine for covering *any*
protocol with this handler. I'd think this is rather hard per the
slight differences in SASL exchanges for different protocols.

> * remove password_encryption parameter and always use default
> encryption as specified for that session in pg_hba.conf

So if user X creates user Y with a password (defined by CREATE USER
PASSWORD) it should by default follow what pg_hba.conf dictates, which
could be pam or gss? That does not look very intuitive to me. The
advantage with the current system is that password creation and
protocol allowed for an authentication are two separate, independent
things, password_encryption being basically a wrapper for CREATE USER.
Mixing both makes things more confusing. If you are willing to move
away from password_encryption, one thing that could be used is just to
extend CREATE USER to be able to enforce the password protocol
associated, that's what the patches on this thread do with PASSWORD
(val USING protocol).

> Which sounds nice, but many users will wish to upgrade their current
> mechanisms from using md5 to scram. How will we update passwords
> slowly, so that different users change from md5 to scram at different
> times? Having to specify the mechanism in the pg_hba.conf makes that
> almost impossible, forcing a big bang approach which subsequently may
> never happen.

At this point comes the possibility to define multiple password types
for one single user instead of rolling multiple roles and renaming
htem.

> As a way of solving that problem, another idea would be to make the
> mechanism session specific depending upon what is stored for a
> particular user. That allows us to have a single pg_hba.conf entry of
> "sasl", and then use md5, scram-256 or future-mechanism on a per user
> basis.

Isn't that specifying multiple users in a single sasl entry in
pg_hba.conf? Once a user is updated, you could just move him from one
line to the other of pg_hba.conf, or use a @file in the hba entry.

> I'm not sure I see a clear way forwards yet, these are just ideas and
> questions to help the discussion.

Thanks, I find the catalog idea interesting. That's hard though per
the potential range of SASL protocols that have likely different needs
in the way messages are exchanged.
-- 
Michael


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