Re: [HACKERS] pgbench: faster version of tpcb-like transaction

[Fabien COELHO]

Hello,

If all the data is in memory and you have a system with fast fsyncs (or 
are running with fsync off, or unlogged tables, or synchronous_commit 
off), then the big bottleneck in pgbench is the amount of back and forth 
between the pgbench program and the backend.


Sure. The throughput of a benchmark depends on a bottleneck which may be 
disk ios, cpu, network, load... depending on the test conditions.


I tested quite a few variants for my PgDay Paris 2017 talk, including PL 
functions, see https://wiki.postgresql.org/wiki/PgDay_Paris_2017.


--
Fabien.


--
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: faster version of tpcb-like transaction

[Tom Lane]

Jeff Janes  writes:
> If all the data is in memory and you have a system with fast fsyncs (or are
> running with fsync off, or unlogged tables, or synchronous_commit off),
> then the big bottleneck in pgbench is the amount of back and forth between
> the pgbench program and the backend.  There are 7 commands per transaction.

Yeah ...

> It is easy to package 5 of those commands into a single PL/pgSQL function,
> with the other two being implicit via the standard auto-commit behavior
> when explicit transactions are not opened.  The attached patch does that,
> under the name tpcb-func.  I first named it tpcb-like-func, but one builtin
> name can't be a prefix or another so that won't work.

I dunno, it seems like this proposal involves jacking up the test case
and driving a completely different one underneath.  There is no reason
to consider that you've improved the benchmark results --- you've just
substituted a different benchmark, one with no historical basis, and
not a lot of field justification either.

> Wanting to measure IPC overhead is a valid thing to do, but
> certainly isn't the most common thing people want to do with pgbench.

I think that's nonsense.  Measuring how fast PG can do client interactions
is EXACTLY what this is about.  Certainly, pushing SQL operations into
server-side functions is a great way to reduce network overhead, but it
has nothing to do with what we choose as a benchmark.

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] More replication race conditions

[Tom Lane]

And *another* replication test race condition just now:

https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=dangomushi=2017-08-26%2019%3A37%3A08

As best I can interpret this, it's pointing out that this bit in
src/test/recovery/t/009_twophase.pl:

$cur_master->psql(
'postgres', "
BEGIN;
CREATE TABLE t_009_tbl2 (id int, msg text);
SAVEPOINT s1;
INSERT INTO t_009_tbl2 VALUES (27, 'issued to ${cur_master_name}');
PREPARE TRANSACTION 'xact_009_13';
-- checkpoint will issue XLOG_STANDBY_LOCK that can conflict with lock
-- held by 'create table' statement
CHECKPOINT;
COMMIT PREPARED 'xact_009_13';");

$cur_standby->psql(
'postgres',
"SELECT count(*) FROM t_009_tbl2",
stdout => \$psql_out);
is($psql_out, '1', "Replay prepared transaction with DDL");

contains exactly no means of ensuring that the master's transaction has
been replayed on the standby before we check for its results.  It's not
real clear why it seems to work 99.99% of the time, because, well, there
isn't any frickin' interlock there ...

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] More replication race conditions

[Noah Misch]

On Fri, Aug 25, 2017 at 12:09:00PM +0200, Petr Jelinek wrote:
> On 24/08/17 19:54, Tom Lane wrote:
> > sungazer just failed with
> > 
> > pg_recvlogical exited with code '256', stdout '' and stderr 
> > 'pg_recvlogical: could not send replication command "START_REPLICATION SLOT 
> > "test_slot" LOGICAL 0/0 ("include-xids" '0', "skip-empty-xacts" '1')": 
> > ERROR:  replication slot "test_slot" is active for PID 8913148
> > pg_recvlogical: disconnected
> > ' at 
> > /home/nm/farm/gcc64/HEAD/pgsql.build/src/test/recovery/../../../src/test/perl/PostgresNode.pm
> >  line 1657.
> > 
> > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=sungazer=2017-08-24%2015%3A16%3A10
> > 
> > Looks like we're still not there on preventing replication startup
> > race conditions.
> 
> Hmm, that looks like "by design" behavior. Slot acquiring will throw
> error if the slot is already used by somebody else (slots use their own
> locking mechanism that does not wait). In this case it seems the
> walsender which was using slot for previous previous step didn't finish
> releasing the slot by the time we start new command. We can work around
> this by changing the test to wait perhaps.

[Action required within three days.  This is a generic notification.]

The above-described topic is currently a PostgreSQL 10 open item.  Simon,
since you committed the patch believed to have created it, you own this open
item.  If some other commit is more relevant or if this does not belong as a
v10 open item, please let us know.  Otherwise, please observe the policy on
open item ownership[1] and send a status update within three calendar days of
this message.  Include a date for your subsequent status update.  Testers may
discover new open items at any time, and I want to plan to get them all fixed
well in advance of shipping v10.  Consequently, I will appreciate your efforts
toward speedy resolution.  Thanks.

[1] 
https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com


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

[HACKERS] Re: Poor cost estimate with interaction between table correlation and partial indexes

[Michael Malis]

Do you think this is a reasonable approach? Should I start working on
a patch based on the solution I described or is there some other
approach I should look into?


-- 
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: faster version of tpcb-like transaction

[Peter Geoghegan]

On Sat, Aug 26, 2017 at 4:59 PM, Jeff Janes  wrote:
> I still get a 2 fold improvement, from 13668 to 27036, when both
> transactions are tested with -M prepared.
>
> I am surprised, I usually haven't seen that much difference for the default
> queries between prepared or not, to the point that I got out of the habit of
> testing with it.  But back when I was testing with and without
> systematically, I did notice that it changed a lot depending on hardware and
> concurrency.  And of course from version to version different bottlenecks
> come and go.

I must admit that I had a similar unpleasant surprise at one point --
"-M prepared" seems to matter *a lot* these days. That's the default
that I'd change, if any.

-- 
Peter Geoghegan


-- 
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: faster version of tpcb-like transaction

[Jeff Janes]

On Sat, Aug 26, 2017 at 4:28 PM, Peter Geoghegan  wrote:

> On Sat, Aug 26, 2017 at 3:53 PM, Jeff Janes  wrote:
> > I get nearly a 3 fold speed up using the new transaction, from 9184 to
> 26383
> > TPS, on 8 CPU machine using scale 50 and:
> >
> > PGOPTIONS="-c synchronous_commit=off" pgbench -c32 -j32 -T60 -b tpcb-like
>
> What about with "-M prepared"? I think that most of us use that
> setting already, especially with CPU-bound workloads.
>

I still get a 2 fold improvement, from 13668 to 27036, when both
transactions are tested with -M prepared.

I am surprised, I usually haven't seen that much difference for the default
queries between prepared or not, to the point that I got out of the habit
of testing with it.  But back when I was testing with and without
systematically, I did notice that it changed a lot depending on hardware
and concurrency.  And of course from version to version different
bottlenecks come and go.

And thanks to Tom for letting me put -M at the end of the command line now.

Cheers,

Jeff

Re: [HACKERS] pgbench: faster version of tpcb-like transaction

[Peter Geoghegan]

On Sat, Aug 26, 2017 at 3:53 PM, Jeff Janes  wrote:
> I get nearly a 3 fold speed up using the new transaction, from 9184 to 26383
> TPS, on 8 CPU machine using scale 50 and:
>
> PGOPTIONS="-c synchronous_commit=off" pgbench -c32 -j32 -T60 -b tpcb-like

What about with "-M prepared"? I think that most of us use that
setting already, especially with CPU-bound workloads.


-- 
Peter Geoghegan


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

[HACKERS] Poor cost estimate with interaction between table correlation and partial indexes

[Michael Malis]

Hi.

I'm looking to get started contributing code to Postgres. A small
issue I'm aware of that I think would make a good first contribution
is a poor cost estimate made due to an interaction between table
correlation and partial indexes. Currently the planner assumes that
when an index is perfectly correlated with a table and a range scan is
performed on the index, all of the table page reads performed by the
index scan except for the first one will be sequential reads. While
this assumption is correct for regular indexes, it is not true for
partial indexes.

The assumption holds for regular indexes because the rows
corresponding to two entries in a regular index that is perfectly
correlated with the table are guaranteed to be next to each other in
the table. On the other hand with a partial index perfectly correlated
with a table, there may be rows in the table in between the two rows
corresponding to two adjacent entries in the index that are not
included in the index because they do not satisfy the partial index
predicate.

To make the cost calculation for this case more accurate, I want to
apply the same estimate as the one currently used to estimate the cost
of a bitmap heap scan. The bitmap heap scan cost estimate applies in
this case because both cases involve reading pages from disk ordered
by the location in the table, but where the pages may not be
consecutive.

For the relevant functions, see cost_index and cost_index_heap_scan in
costsize.c.

Thanks,
Michael


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

[HACKERS] pgbench: faster version of tpcb-like transaction

[Jeff Janes]

If all the data is in memory and you have a system with fast fsyncs (or are
running with fsync off, or unlogged tables, or synchronous_commit off),
then the big bottleneck in pgbench is the amount of back and forth between
the pgbench program and the backend.  There are 7 commands per transaction.

It is easy to package 5 of those commands into a single PL/pgSQL function,
with the other two being implicit via the standard auto-commit behavior
when explicit transactions are not opened.  The attached patch does that,
under the name tpcb-func.  I first named it tpcb-like-func, but one builtin
name can't be a prefix or another so that won't work.

It creates the function unconditionally during -i, because there is no way
to know if the run-time will end up using it or not.  I think this is OK.
PL/pgSQL is installed by default in all supported versions. If someone has
gone through the bother of uninstalling it, I don't see a need to
accommodate them here.

I get nearly a 3 fold speed up using the new transaction, from 9184
to 26383 TPS, on 8 CPU machine using scale 50 and:

PGOPTIONS="-c synchronous_commit=off" pgbench -c32 -j32 -T60 -b tpcb-like

I think this should be committed as a built-in, not just a user-defined
transaction, because I would like to see it widely used.  In fact, if it
weren't for historical consistency I would say it should be the default
transaction.  Wanting to measure IPC overhead is a valid thing to do, but
certainly isn't the most common thing people want to do with pgbench.  If a
user is limited by IO, it wouldn't matter which transaction they use, and
if they are not limited by IO then this transaction is more likely to be
the right one for them than the current default one transaction is.

Also, as a user-defined transaction with -f, you have to go out of your way
to create the function (no "-i" support) and to make sure :scale gets set
correctly during runs (as it won't be automatically read from
pgbench_branches table, you have manually give -D).

Cheers,

Jeff


pgbench_function_v1.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] subscription worker signalling wal writer too much

[Jeff Janes]

On Mon, Jul 3, 2017 at 8:26 PM, Jeff Janes  wrote:

> On Sat, Jun 24, 2017 at 5:09 PM, Andres Freund  wrote:
>
>> On 2017-06-15 15:06:43 -0700, Jeff Janes wrote:
>>
>>
>>
>
>> > Wouldn't it
>> > better, and much simpler, just to have reverted the first change once
>> the
>> > second one was done?
>>
>> I think both can actually happen independently, no? It's pretty common
>> for the page lsn to be *older* than the corresponding commit.  In fact
>> you'll always hit that case unless there's also concurrent writes also
>> touching said page.
>>
>
> That is true, but I don't see any residual regression when going from
> pre-db76b1efbbab2441428 to 7975c5e0a992ae9a4-with-my-patch applied.  So I
> think the hint bit issue only shows up when hitting the same page
> aggressively, in which case the LSN does get advanced quickly enough that
> db76b1efbba solves it.  Perhaps a different test case could show that
> issue.  I also didn't see any improvement from the original 4de82f7d7c50a81e,
> so maybe 8 CPUs just isn't enough to detect the problem with setting
> hint-buts with permanent tables with synchronous_commit=false.
>

I've refined my benchmarking, using a smaller scaling factor (8, same as my
CPU count) and running the tests for longer. The smaller size means there
are more intensive updates on individual pgbench_accounts pages, and the
longer run times allows be unset hint bits to build up for longer (usually
I like short test runs with a large number of randomized repetitions, but
that doesn't work well here).  Since the scale factor is the same as the
number of CPUs, I've bumped the thread count so that it is more likely
someone will choose a non-contended value of pgbench_branches.bid to update
at any given moment.

pgbench -c16 -j16 -T1200 -M prepared -b tpcb-func --startup='set
synchronous_commit=false'

This testing did show the importance of waking up the wal writer so that
hint bits can be set:

commit TPS
cfafd8beadcd6f 22200.48
cfafd8beadcd6f_nowake 30766.83
median of 14 runs reported, p-val on difference of means is 2e-22.

Where nowake is a hackish disabling of the wake up introduced in
4de82f7d7c50a8,
forward ported to 9.6 branch.  (I still wake it if is is doing the big
sleep, I just took out the part that wake it up even from small sleeps)

So my revised benchmark is capable of detecting this effect, even with only
8 CPUs.

When I move to next commit where we set hint bits as long as the page was
re-dirtied after, get these numbers:

db76b1efbbab24 30742.69
db76b1efbbab24_nowake 31072.97

The difference between these is not statistically different, nor is the
difference between them and cfafd8beadcd6f.

So I think the logic you introduced in db76b1efbbab24 captures all the gain
there is to be captured, and 4de82f7d7c50a8 can be reverted.  The only
reason to wake up the wal writer is if it is taking the big sleep.

Of course, I can't prove a negative.  There could always be some test case
which demonstrates that 4de82f7d7c50a8 still matters in spite of
db76b1efbbab24.
So to be ultra-conservative, attached is a patch which should preserve all
wake-ups other than the ones known to be useless.

7975c5e0a 27810.66
7975c5e0a_patched 30821.41

So 7975c5 causes a significant regression (10% regression, p-val 7e-16 on
difference of the means).  It repeatedly wakes the wal-writer up to flush a
page which the wal-writer simply refuses to flush.  This can't serve any
purpose.  My patch still wakes it up to write the page if it has not been
written yet, and also still wakes it to flush a range of pages of
WalWriterFlushAfter is met.  But it just doesn't wake it up to flush a page
which has already been written and will not get flushed.

I'd prefer the code-simplifying change of just not waking it up anymore
except for when it is doing the big sleep, but I can't reliably detect a
performance difference between that and the more conservative change posted
here.

Cheers,

Jeff


change_walwriter_wakeup_v2.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] Patch: add --if-exists to pg_recvlogical

[Rosser Schwarz]

On Fri, Aug 25, 2017 at 12:34 PM, Robert Haas  wrote:

> On Sun, May 21, 2017 at 3:04 PM, Rosser Schwarz
>  wrote:
> > While doing some scripting around pg_recvlogical at $work, I found a need
> > for $subject. Attached, find a patch to that effect...

Please add this to commitfest.postgresql.org


Done, thanks!

https://commitfest.postgresql.org/14/1256/

-- 
:wq

[HACKERS] Back-branch release notes up for review

[Tom Lane]

I've drafted notes for next week's brown-paper-bag releases.
If you want to review, see
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=f1b10496a55a64b2872633850e55a2cd9d1c9108

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] [COMMITTERS] pgsql: pg_test_timing: Add NLS

[Peter Eisentraut]

On 7/6/17 14:56, Alvaro Herrera wrote:
> We (well, Carlos Chapi, who's doing the translation work now) just
> noticed that this has a bug in this line
> 
> +   printf("%6s   %10s %10s\n", _("< us"), _("% of total"), _("count"));
> 
> _() marks the strings with the c-string flag, which means that the
> %-specifiers are checked by gettext, but the % in the third literal is
> not a printf specifier.  So there's no correct way to write the
> translation.  We need to use a different xgettext trigger there, one
> that doesn't set c-format, but I don't know what.

I have fixed this.

-- 
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] Variable substitution in psql backtick expansion

[Tom Lane]

Fabien COELHO  writes:
> Spending developer time to write code for the hypothetical someone running 
> a psql version 11 linked to a libpq < 7.4, if it can even link, does not 
> look like a very good investment... Anyway, here is required the update.

The question is the server's version, not libpq.  Modern psql does still
talk to ancient servers (I tried 11devel against 7.2 just now, to be
sure).  The stuff in describe.c may not work well, but basic functionality
is there and I don't want to break it.

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] psql - add ability to test whether a variable exists

[Fabien COELHO]

Here is a version with the :{?varname} syntax.


It looks much better for me.


I'll admit that it looks better to me as well:-)

--
Fabien.


--
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] Variable substitution in psql backtick expansion

[Fabien COELHO]

Hello Tom,


I think you are taking unreasonable shortcuts here:

+   SetVariable(pset.vars, "SERVER_VERSION_NAME", PQparameterStatus(pset.db, 
"server_version"));

The existing code in connection_warnings() does this:

   const char *server_version;

   /* Try to get full text form, might include "devel" etc */
   server_version = PQparameterStatus(pset.db, "server_version");
   /* Otherwise fall back on pset.sversion */
   if (!server_version)
   {
   formatPGVersionNumber(pset.sversion, true,
 sverbuf, sizeof(sverbuf));
   server_version = sverbuf;
   }

and I think you should duplicate that logic verbatim.  Now admittedly,
server_version has been available for a long time, so that this might
never matter in practice.  But we shouldn't be doing this one way
in one place and differently somewhere else.


Hmmm. I think this code may have been justified around version 6/7. This 
code could probably be removed: according to the online documentation, 
"server_version" seems supported at least back to 7.4. Greping old sources 
suggest that it is not implemented in 7.3, though.


Spending developer time to write code for the hypothetical someone running 
a psql version 11 linked to a libpq < 7.4, if it can even link, does not 
look like a very good investment... Anyway, here is required the update.


--
Fabien.diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index c592eda..c611f39 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -3688,6 +3688,18 @@ bar
   
 
   
+SERVER_VERSION_NAME
+SERVER_VERSION_NUM
+
+
+These variables are set when connected to reflects the server's version
+both in short name (eg "9.6.2", "10.0") and number (eg 90602, 10).
+They can be changed or unset.
+
+
+  
+
+  
 SINGLELINE
 
 
@@ -3733,10 +3745,13 @@ bar
 
   
 VERSION
+VERSION_NAME
+VERSION_NUM
 
 
-This variable is set at program start-up to
-reflect psql's version.  It can be changed or unset.
+These variables are set at program start-up to reflect
+psql's version in verbose, short name ("10.0")
+and number (10).  They can be changed or unset.
 
 
   
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 96f3a40..b58d8b6 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -3337,6 +3337,9 @@ checkWin32Codepage(void)
 void
 SyncVariables(void)
 {
+	char vbuf[32];
+	const char *server_version;
+
 	/* get stuff from connection */
 	pset.encoding = PQclientEncoding(pset.db);
 	pset.popt.topt.encoding = pset.encoding;
@@ -3348,6 +3351,19 @@ SyncVariables(void)
 	SetVariable(pset.vars, "PORT", PQport(pset.db));
 	SetVariable(pset.vars, "ENCODING", pg_encoding_to_char(pset.encoding));
 
+	/* server version information */
+	server_version = PQparameterStatus(pset.db, "server_version");
+	/* Otherwise fall back on pset.sversion for libpq < 7.4 */
+	if (!server_version)
+	{
+		formatPGVersionNumber(pset.sversion, true, vbuf, sizeof(vbuf));
+		server_version = vbuf;
+	}
+	SetVariable(pset.vars, "SERVER_VERSION_NAME", server_version);
+
+	snprintf(vbuf, sizeof(vbuf), "%d", pset.sversion);
+	SetVariable(pset.vars, "SERVER_VERSION_NUM", vbuf);
+
 	/* send stuff to it, too */
 	PQsetErrorVerbosity(pset.db, pset.verbosity);
 	PQsetErrorContextVisibility(pset.db, pset.show_context);
@@ -3366,6 +3382,8 @@ UnsyncVariables(void)
 	SetVariable(pset.vars, "HOST", NULL);
 	SetVariable(pset.vars, "PORT", NULL);
 	SetVariable(pset.vars, "ENCODING", NULL);
+	SetVariable(pset.vars, "SERVER_VERSION_NAME", NULL);
+	SetVariable(pset.vars, "SERVER_VERSION_NUM", NULL);
 }
 
 
diff --git a/src/bin/psql/startup.c b/src/bin/psql/startup.c
index 7f76797..4065539 100644
--- a/src/bin/psql/startup.c
+++ b/src/bin/psql/startup.c
@@ -161,6 +161,8 @@ main(int argc, char *argv[])
 	EstablishVariableSpace();
 
 	SetVariable(pset.vars, "VERSION", PG_VERSION_STR);
+	SetVariable(pset.vars, "VERSION_NAME", PG_VERSION);
+	SetVariable(pset.vars, "VERSION_NUM", CppAsString2(PG_VERSION_NUM));
 
 	/* Default values for variables (that don't match the result of \unset) */
 	SetVariableBool(pset.vars, "AUTOCOMMIT");

-- 
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] psql - add ability to test whether a variable exists

[Corey Huinker]

On Sat, Aug 26, 2017 at 2:09 PM, Pavel Stehule 
wrote:

>
>
> 2017-08-26 19:55 GMT+02:00 Fabien COELHO :
>
>>
>> Any colon prefixed syntax can be made to work because it is enough for
>>> the lexer to detect and handle... so
>>>
>>>  :{defined varname}
>>>
>>> may be an option, although I do not like the space much because it adds
>>> some fuzzyness in the lexer which has to process it. It is probably doable,
>>> though. I like having a "?" because there is a question. Other
>>> suggestions somehow in line with your proposal could be
>>>  :{?varname}
>>>  :{varname?}
>>> what do you think?
>>>
>>
>> Here is a version with the :{?varname} syntax.
>
>
> It looks much better for me.
>
> Regards
>
> Pavel
>

+1. Glad to have this feature. Any of the proposed syntaxes look good to
me, with a slight preference for {?var}.

Re: [HACKERS] psql - add ability to test whether a variable exists

[Pavel Stehule]

2017-08-26 19:55 GMT+02:00 Fabien COELHO :

>
> Any colon prefixed syntax can be made to work because it is enough for the
>> lexer to detect and handle... so
>>
>>  :{defined varname}
>>
>> may be an option, although I do not like the space much because it adds
>> some fuzzyness in the lexer which has to process it. It is probably doable,
>> though. I like having a "?" because there is a question. Other
>> suggestions somehow in line with your proposal could be
>>  :{?varname}
>>  :{varname?}
>> what do you think?
>>
>
> Here is a version with the :{?varname} syntax.


It looks much better for me.

Regards

Pavel

>
>
> --
> Fabien.

Re: [HACKERS] psql - add ability to test whether a variable exists

[Fabien COELHO]

Any colon prefixed syntax can be made to work because it is enough for the 
lexer to detect and handle... so


 :{defined varname}

may be an option, although I do not like the space much because it adds some 
fuzzyness in the lexer which has to process it. It is probably doable, 
though. I like having a "?" because there is a question. Other

suggestions somehow in line with your proposal could be
 :{?varname}
 :{varname?}
what do you think?


Here is a version with the :{?varname} syntax.

--
Fabien.diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index c592eda..58f8e9a 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -781,6 +781,10 @@ testdb=
 The forms :'variable_name' and
 :"variable_name" described there
 work as well.
+The :{?variable_name} syntax allows
+to test whether a variable is defined. It is substituted by
+TRUE or FALSE.
+Escaping the colon with a backslash protects it from substitution.
 
 
 
@@ -3813,6 +3817,12 @@ testdb= INSERT INTO my_table VALUES (:'content');
 
 
 
+The :{?name} special syntax returns TRUE
+or FALSE depending on whether the variable exists or not, thus is
+always substituted, unless the colon is backslash-escaped.
+
+
+
 The colon syntax for variables is standard SQL for
 embedded query languages, such as ECPG.
 The colon syntaxes for array slices and type casts are
diff --git a/src/bin/psql/psqlscanslash.l b/src/bin/psql/psqlscanslash.l
index db7a1b9..9a53cb3 100644
--- a/src/bin/psql/psqlscanslash.l
+++ b/src/bin/psql/psqlscanslash.l
@@ -281,6 +281,10 @@ other			.
 	unquoted_option_chars = 0;
 }
 
+:\{\?{variable_char}+\}	{
+	psqlscan_test_variable(cur_state, yytext, yyleng);
+}
+
 :'{variable_char}*	{
 	/* Throw back everything but the colon */
 	yyless(1);
@@ -295,6 +299,20 @@ other			.
 	ECHO;
 }
 
+:\{\?{variable_char}*	{
+	/* Throw back everything but the colon */
+	yyless(1);
+	unquoted_option_chars++;
+	ECHO;
+}
+
+:\{		{
+	/* Throw back everything but the colon */
+	yyless(1);
+	unquoted_option_chars++;
+	ECHO;
+}
+
 {other}			{
 	unquoted_option_chars++;
 	ECHO;
diff --git a/src/fe_utils/psqlscan.l b/src/fe_utils/psqlscan.l
index 27689d7..4375142a 100644
--- a/src/fe_utils/psqlscan.l
+++ b/src/fe_utils/psqlscan.l
@@ -745,9 +745,13 @@ other			.
 			 PQUOTE_SQL_IDENT);
 }
 
+:\{\?{variable_char}+\}	{
+	psqlscan_test_variable(cur_state, yytext, yyleng);
+}
+
 	/*
 	 * These rules just avoid the need for scanner backup if one of the
-	 * two rules above fails to match completely.
+	 * three rules above fails to match completely.
 	 */
 
 :'{variable_char}*	{
@@ -762,6 +766,17 @@ other			.
 	ECHO;
 }
 
+:\{\?{variable_char}*	{
+	/* Throw back everything but the colon */
+	yyless(1);
+	ECHO;
+}
+:\{	{
+	/* Throw back everything but the colon */
+	yyless(1);
+	ECHO;
+}
+
 	/*
 	 * Back to backend-compatible rules.
 	 */
@@ -1442,3 +1457,28 @@ psqlscan_escape_variable(PsqlScanState state, const char *txt, int len,
 		psqlscan_emit(state, txt, len);
 	}
 }
+
+void
+psqlscan_test_variable(PsqlScanState state, const char *txt, int len)
+{
+	char	*varname;
+	char	*value;
+
+	varname = psqlscan_extract_substring(state, txt + 3, len - 4);
+	if (state->callbacks->get_variable)
+		value = state->callbacks->get_variable(varname, PQUOTE_PLAIN,
+			   state->cb_passthrough);
+	else
+		value = NULL;
+	free(varname);
+
+	if (value != NULL)
+	{
+		psqlscan_emit(state, "TRUE", 4);
+		free(value);
+	}
+	else
+	{
+		psqlscan_emit(state, "FALSE", 5);
+	}
+}
diff --git a/src/include/fe_utils/psqlscan_int.h b/src/include/fe_utils/psqlscan_int.h
index c70ff29..e9b3517 100644
--- a/src/include/fe_utils/psqlscan_int.h
+++ b/src/include/fe_utils/psqlscan_int.h
@@ -142,5 +142,7 @@ extern char *psqlscan_extract_substring(PsqlScanState state,
 extern void psqlscan_escape_variable(PsqlScanState state,
 		 const char *txt, int len,
 		 PsqlScanQuoteType quote);
+extern void psqlscan_test_variable(PsqlScanState state,
+   const char *txt, int len);
 
 #endif			/* PSQLSCAN_INT_H */
diff --git a/src/test/regress/expected/psql.out b/src/test/regress/expected/psql.out
index 4aaf4c1..2b2f23b 100644
--- a/src/test/regress/expected/psql.out
+++ b/src/test/regress/expected/psql.out
@@ -2929,6 +2929,32 @@ bar 'bar' "bar"
 	\echo 'should print #8-1'
 should print #8-1
 \endif
+-- :{?...} defined variable test
+\set i 1
+\if :{?i}
+  \echo '#9-1 ok, variable i is defined'
+#9-1 ok, variable i is defined
+\else
+  \echo 'should not print #9-2'
+\endif
+\if :{?no_such_variable}
+  \echo 'should not print #10-1'
+\else
+  \echo '#10-2 ok, variable no_such_variable is not defined'
+#10-2 ok, variable no_such_variable is not defined
+\endif
+SELECT :{?i} AS i_is_defined;
+ i_is_defined 
+--
+ t

Re: [HACKERS] Build failure on thrips

[Michael Meskes]

> woodlouse does not do ecpg-check, and Tom is right anyway. I need to 
> get a large sign nailed to my forehead that says "Postgres is C, and
> declarations must come first."

I didn't notice either. I guess /me is getting rusty, sigh.

Michael
-- 
Michael Meskes
Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org)
Meskes at (Debian|Postgresql) dot Org
Jabber: michael at xmpp dot meskes dot org
VfL Borussia! Força Barça! SF 49ers! Use Debian GNU/Linux, PostgreSQL


-- 
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] Build failure on thrips

[Michael Meskes]

> Unless someone has a better idea, I'd suggest working around
> that like so:
> 
> #ifdef WIN32
> #ifdef _MSC_VER/* requires MSVC */
>    _configthreadlocale(_ENABLE_PER_THREAD_LOCALE);
> #endif
> #endif

Let's try, committed, thanks again Tom.

Michael
-- 
Michael Meskes
Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org)
Meskes at (Debian|Postgresql) dot Org
Jabber: michael at xmpp dot meskes dot org
VfL Borussia! Força Barça! SF 49ers! Use Debian GNU/Linux, PostgreSQL


-- 
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] Build failure on thrips

[Christian Ullrich]

* Michael Meskes wrote:

>> It's not just thrips.  Of the Windows machines that have reported in
>> since
>> that commit, only currawong and baiji passed.  Although lorikeet,
>> woodlouse, bowerbird, and brolga are showing green, this proves
>> nothing
>> because they're all configured to skip the ecpg check (... I wonder
>> why).
> 
> I would assume it works on woodlouse as the patch has been submitted by
> woodlouse's owner.

woodlouse does not do ecpg-check, and Tom is right anyway. I need to get a 
large sign nailed to my forehead that says "Postgres is C, and declarations 
must come first."

I'm not sure, and not able to find out right this moment, why thrips' compiler 
does not accept this code now, because I tested it on thrips (and woodlouse, 
IIRC) before I sent it in. 

-- 
Christian

-- 
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] Variable substitution in psql backtick expansion

[Tom Lane]

Fabien COELHO  writes:
> So basically the only thing needed from Robert & you seems to change 
> "11.0" to "11devel", which is fine with me.
> The attached v5 does that.

I think you are taking unreasonable shortcuts here:

+   SetVariable(pset.vars, "SERVER_VERSION_NAME", 
PQparameterStatus(pset.db, "server_version"));

The existing code in connection_warnings() does this:

const char *server_version;

/* Try to get full text form, might include "devel" etc */
server_version = PQparameterStatus(pset.db, "server_version");
/* Otherwise fall back on pset.sversion */
if (!server_version)
{
formatPGVersionNumber(pset.sversion, true,
  sverbuf, sizeof(sverbuf));
server_version = sverbuf;
}

and I think you should duplicate that logic verbatim.  Now admittedly,
server_version has been available for a long time, so that this might
never matter in practice.  But we shouldn't be doing this one way
in one place and differently somewhere else.

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] Build failure on thrips

[Michael Meskes]

> It's not just thrips.  Of the Windows machines that have reported in
> since
> that commit, only currawong and baiji passed.  Although lorikeet,
> woodlouse, bowerbird, and brolga are showing green, this proves
> nothing
> because they're all configured to skip the ecpg check (... I wonder
> why).

I would assume it works on woodlouse as the patch has been submitted by
woodlouse's owner.

Michael
-- 
Michael Meskes
Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org)
Meskes at (Debian|Postgresql) dot Org
Jabber: michael at xmpp dot meskes dot org
VfL Borussia! Força Barça! SF 49ers! Use Debian GNU/Linux, PostgreSQL


-- 
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] Variable substitution in psql backtick expansion

[Fabien COELHO]

Hello Tom,


I understand that you would prefer VERSION_NAME to show something like
   "11devel, server 9.6.4"



No, that's not what I said.  I'm just complaining that as the patch stands
it will set SERVER_NAME to "11.0", where I think it should say "11devel"
(as of today).


Ok.


  [...]
  VERSION "PostgreSQL 11devel on ..."
  CLIENT_VERSION_NAME "11devel"
  CLIENT_VERSION_NUM 11


This kind of inconsistencies is hard for human memory:-(


or just leaving "CLIENT" implicit for all of these variables:

  VERSION "PostgreSQL 11devel on ..."
  VERSION_NAME "11devel"
  VERSION_NUM 11


That is already what the patch does, because of the VERSION precedent.


Robert seems to prefer the last of those, and that'd be fine with me.
(Note that CLIENT is ambiguous anyway: does it mean psql itself, or
libpq?)


Hmmm. Indeed.


   SERVER_VERSION_NAME "9.6.4"
   SERVER_VERSION_NUM 090604


I'm on board with this, except I don't think we should have any leading
zero in the numeric form.  There are contexts where somebody might think
that means octal.


Indeed. The implementation already does this, I just typed it without 
checking.


So basically the only thing needed from Robert & you seems to change 
"11.0" to "11devel", which is fine with me.


The attached v5 does that.

--
Fabien.diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index c592eda..c611f39 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -3688,6 +3688,18 @@ bar
   
 
   
+SERVER_VERSION_NAME
+SERVER_VERSION_NUM
+
+
+These variables are set when connected to reflects the server's version
+both in short name (eg "9.6.2", "10.0") and number (eg 90602, 10).
+They can be changed or unset.
+
+
+  
+
+  
 SINGLELINE
 
 
@@ -3733,10 +3745,13 @@ bar
 
   
 VERSION
+VERSION_NAME
+VERSION_NUM
 
 
-This variable is set at program start-up to
-reflect psql's version.  It can be changed or unset.
+These variables are set at program start-up to reflect
+psql's version in verbose, short name ("10.0")
+and number (10).  They can be changed or unset.
 
 
   
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 96f3a40..3c1924e 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -3337,6 +3337,8 @@ checkWin32Codepage(void)
 void
 SyncVariables(void)
 {
+	char vbuf[32];
+
 	/* get stuff from connection */
 	pset.encoding = PQclientEncoding(pset.db);
 	pset.popt.topt.encoding = pset.encoding;
@@ -3348,6 +3350,11 @@ SyncVariables(void)
 	SetVariable(pset.vars, "PORT", PQport(pset.db));
 	SetVariable(pset.vars, "ENCODING", pg_encoding_to_char(pset.encoding));
 
+	/* server version information */
+	SetVariable(pset.vars, "SERVER_VERSION_NAME", PQparameterStatus(pset.db, "server_version"));
+	snprintf(vbuf, sizeof(vbuf), "%d", pset.sversion);
+	SetVariable(pset.vars, "SERVER_VERSION_NUM", vbuf);
+
 	/* send stuff to it, too */
 	PQsetErrorVerbosity(pset.db, pset.verbosity);
 	PQsetErrorContextVisibility(pset.db, pset.show_context);
@@ -3366,6 +3373,8 @@ UnsyncVariables(void)
 	SetVariable(pset.vars, "HOST", NULL);
 	SetVariable(pset.vars, "PORT", NULL);
 	SetVariable(pset.vars, "ENCODING", NULL);
+	SetVariable(pset.vars, "SERVER_VERSION_NAME", NULL);
+	SetVariable(pset.vars, "SERVER_VERSION_NUM", NULL);
 }
 
 
diff --git a/src/bin/psql/startup.c b/src/bin/psql/startup.c
index 7f76797..4065539 100644
--- a/src/bin/psql/startup.c
+++ b/src/bin/psql/startup.c
@@ -161,6 +161,8 @@ main(int argc, char *argv[])
 	EstablishVariableSpace();
 
 	SetVariable(pset.vars, "VERSION", PG_VERSION_STR);
+	SetVariable(pset.vars, "VERSION_NAME", PG_VERSION);
+	SetVariable(pset.vars, "VERSION_NUM", CppAsString2(PG_VERSION_NUM));
 
 	/* Default values for variables (that don't match the result of \unset) */
 	SetVariableBool(pset.vars, "AUTOCOMMIT");

-- 
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] Build failure on thrips

[Tom Lane]

I wrote:
> thrips is showing some weird syntax errors that I suspect mean it's
> misinterpreting "_configthreadlocale(_ENABLE_PER_THREAD_LOCALE);"
> as a declaration.

Actually, looking closer, isn't the problem that you've stuck a
function call into the midst of declarations?  Some compilers
will allow that, but it's not valid C90.  You probably just need
to move the call down past "EXEC SQL END DECLARE SECTION".

> So what it looks like to me is that either that function isn't
> available on all Windows versions, or there's some other header
> you need to include to get it on some versions.

Microsoft's own documentation contradicts that:
https://msdn.microsoft.com/en-us/library/26c0tb7x(v=vs.80).aspx

So I'm not sure why the undefined-function results on frogmouth
and jacana, although I notice they are using gcc not MSVC.
Unless someone has a better idea, I'd suggest working around
that like so:

#ifdef WIN32
#ifdef _MSC_VER/* requires MSVC */
   _configthreadlocale(_ENABLE_PER_THREAD_LOCALE);
#endif
#endif

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] Build failure on thrips

[Tom Lane]

Michael Meskes  writes:
> The bug fix I committed for fixing setlocale in ECPG test cases on Windows
> seems to work nicely on all buildfarm animals except thrips. Could anyone with
> access to a similar systems please check what's going on or send the
> precompiled file that creates the compilation errors? 

It's not just thrips.  Of the Windows machines that have reported in since
that commit, only currawong and baiji passed.  Although lorikeet,
woodlouse, bowerbird, and brolga are showing green, this proves nothing
because they're all configured to skip the ecpg check (... I wonder why).

In the rest, we have

frogmouth:
thread_implicit.pgc: In function 'test_thread':
thread_implicit.pgc:104:2: warning: implicit declaration of function 
'_configthreadlocale'
thread_implicit.pgc:104:22: error: '_ENABLE_PER_THREAD_LOCALE' undeclared 
(first use in this function)

jacana:
Aug 26 09:06:40 thread_implicit.o: In function `test_thread':
Aug 26 09:06:40 
c:\mingw\msys\1.0\home\pgrunner\bf\root\HEAD\pgsql.build\src\interfaces\ecpg\test\thread/thread_implicit.pgc:104:
 undefined reference to `_configthreadlocale'

thrips is showing some weird syntax errors that I suspect mean it's
misinterpreting "_configthreadlocale(_ENABLE_PER_THREAD_LOCALE);"
as a declaration.

So what it looks like to me is that either that function isn't
available on all Windows versions, or there's some other header
you need to include to get it on some versions.

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] psql - add ability to test whether a variable exists

[Fabien COELHO]

Hello Pavel,


As a follow-up to the \if patch by Corey Huinker, here is a proposal to
allow testing whether a client-side variable exists in psql.

The syntax is as ugly as the current :'var' and :"var" things, but ISTM
that this is the only simple option to have a working SQL-compatible syntax
with both client-side substitution and server side execution. See the
second example below.


It is really ugly


Yes, I agree:-)


- the ? symbol is not used in pair usually - so it is
much more visible - it is bad readable.


Yep.


Maybe some other syntax: :{fx xxx} .. where fx can be one from more
possible operators ? ! ...


Any colon prefixed syntax can be made to work because it is enough for 
the lexer to detect and handle... so


  :{defined varname}

may be an option, although I do not like the space much because it adds 
some fuzzyness in the lexer which has to process it. It is probably 
doable, though. I like having a "?" because there is a question. Other

suggestions somehow in line with your proposal could be
  :{?varname}
  :{varname?}
what do you think?


The other option would be to have some special keyword syntax, say
"defined var", but then it would have to be parsed client side, and how to
do that in an SQL expression is unclear, and moreover it would not look
right in an SQL expression. If it would look like a function call, say
"defined('var')", it would potentially interact with existing server-side
user-defined functions, which is pretty undesirable. Hence the :?...?
proposal above which is limited to variable subsitution syntax.


should not be solved by introduction \ifdef ?


This would be a client side only non extendable option: you can only test 
one variable at a time, you cannot say "NOT" or have to do \ifndef... CPP 
started like that and ended with #if bool-expr-with-defined in the end.


The idea is to extend the newly added \if with client-side SQL-compatible 
expression syntax, and that the same syntax could be used server side with 
select, eg:


  -- client side
  \let i :j + 1
  -- server side
  SELECT :j + 1 AS i \gset
  -- client-side conditional
  \if NOT :j > 1 OR ...

The colon prefixed substitution syntax already works for server side,
but there is no way to check whether a variable exists which is 
compatible with that, hence this patch.


Pgbench has expressions with patches to improve it, especially adding 
boolean operators. I think that the simplest plan is, when stabalized, to 
move the parser & executir to fe_utils and to use it from both psql et 
pgbench. Also I plan to add \if to pgbench, possibly by factoring some of 
the code from psql in fe_utils as well because it would help with 
benchmarks.


Now given the patch queue length and speed I'm not even thinking of 
starting doing all this.


--
Fabien.


--
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] Variable substitution in psql backtick expansion

[Tom Lane]

Fabien COELHO  writes:
>> OK, but if human-friendly display is the use-case then it ought to
>> duplicate what psql itself would print in, eg, the startup message about
>> server version mismatch.  The v4 patch does not, in particular it neglects
>> PQparameterStatus(pset.db, "server_version").  This would result in
>> printing, eg, "11.0" when the user would likely rather see "11devel".

> I understand that you would prefer VERSION_NAME to show something like
>"11devel, server 9.6.4"

No, that's not what I said.  I'm just complaining that as the patch stands
it will set SERVER_NAME to "11.0", where I think it should say "11devel"
(as of today).

> In summary, my prefered option is to have:
>CLIENT_VERSION "PostgreSQL 11devel on ..."
>CLIENT_VERSION_NAME "11devel"
>CLIENT_VERSION_NUM 11

I don't think we want to drop :VERSION; that would accomplish little
beyond breaking existing scripts.  Plausible choices include duplicating
it, like:

   VERSION "PostgreSQL 11devel on ..."
   CLIENT_VERSION "PostgreSQL 11devel on ..."
   CLIENT_VERSION_NAME "11devel"
   CLIENT_VERSION_NUM 11

or just ignoring the discrepancy:

   VERSION "PostgreSQL 11devel on ..."
   CLIENT_VERSION_NAME "11devel"
   CLIENT_VERSION_NUM 11

or just leaving "CLIENT" implicit for all of these variables:

   VERSION "PostgreSQL 11devel on ..."
   VERSION_NAME "11devel"
   VERSION_NUM 11

Robert seems to prefer the last of those, and that'd be fine with me.
(Note that CLIENT is ambiguous anyway: does it mean psql itself, or
libpq?)

>SERVER_VERSION_NAME "9.6.4"
>SERVER_VERSION_NUM 090604

I'm on board with this, except I don't think we should have any leading
zero in the numeric form.  There are contexts where somebody might think
that means octal.

regards, tom lane


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

Re: [HACKERS] PATCH: multivariate histograms and MCV lists

[Tomas Vondra]

On 08/17/2017 12:06 PM, Adrien Nayrat wrote:>
> Hello,
> 
> There is no check of "statistics type/kind" in
> pg_stats_ext_mcvlist_items and pg_histogram_buckets.
> 
> select stxname,stxkind from pg_statistic_ext ; stxname  | stxkind 
> ---+- stts3 | {h} stts2 | {m}
> 
> So you can call :
> 
> SELECT * FROM pg_mcv_list_items((SELECT oid FROM pg_statistic_ext
> WHERE stxname = 'stts3'));
> 
> SELECT * FROM pg_histogram_buckets((SELECT oid FROM pg_statistic_ext
> WHERE stxname = 'stts2'), 0);
> 
> Both crashes.
> 

Thanks for the report, this is clearly a bug. I don't think we need to
test the stxkind, but rather a missing check that the requested type is
actually built.

> Unfotunately, I don't have the knowledge to produce a patch :/
> 
> Small fix in documentation, patch attached.
> 

Thanks, will fix.

regards

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

[HACKERS] hash partitioning based on v10Beta2

[yang...@highgo.com]

Hi all,

Now we have had the range / list partition, but hash partitioning is not 
implemented yet. 
Attached is a POC patch based on the v10Beta2 to add the hash partitioning 
feature. 
Although we will need more discussions about the syntax and other 
specifications before going ahead the project, 
but I think this runnable code might help to discuss what and how we implement 
this.

Description

The hash partition's implement is on the basis of the original range / list 
partition,and using similar syntax.

To create a partitioned table ,use:

CREATE TABLE h (id int) PARTITION BY HASH(id);

The partitioning key supports only one value, and I think the partition key can 
support multiple values, 
which may be difficult to implement when querying, but it is not impossible.

A partition table can be create as bellow:

 CREATE TABLE h1 PARTITION OF h;
 CREATE TABLE h2 PARTITION OF h;
 CREATE TABLE h3 PARTITION OF h;
 
FOR VALUES clause cannot be used, and the partition bound is calclulated 
automatically as partition index of single integer value.

An inserted record is stored in a partition whose index equals 
DatumGetUInt32(OidFunctionCall1(lookup_type_cache(key->parttypid[0], 
TYPECACHE_HASH_PROC)->hash_proc, values[0])) % nparts /* Number of partitions */
;
In the above example, this is 
DatumGetUInt32(OidFunctionCall1(lookup_type_cache(key->parttypid[0], 
TYPECACHE_HASH_PROC)->hash_proc, id)) % 3;

postgres=# insert into h select generate_series(1,20);
INSERT 0 20
postgres=# select tableoid::regclass,* from h;
 tableoid | id 
--+
 h1   |  3
 h1   |  5
 h1   | 17
 h1   | 19
 h2   |  2
 h2   |  6
 h2   |  7
 h2   | 11
 h2   | 12
 h2   | 14
 h2   | 15
 h2   | 18
 h2   | 20
 h3   |  1
 h3   |  4
 h3   |  8
 h3   |  9
 h3   | 10
 h3   | 13
 h3   | 16
(20 rows)

The number of partitions here can be dynamically added, and if a new partition 
is created, the number of partitions changes, the calculated target partitions 
will change, and the same data is not reasonable in different partitions,So you 
need to re-calculate the existing data and insert the target partition when you 
create a new partition.

postgres=# create table h4 partition of h;
CREATE TABLE
postgres=# select tableoid::regclass,* from h;
 tableoid | id 
--+
 h1   |  5
 h1   | 17
 h1   | 19
 h1   |  6
 h1   | 12
 h1   |  8
 h1   | 13
 h2   | 11
 h2   | 14
 h3   |  1
 h3   |  9
 h3   |  2
 h3   | 15
 h4   |  3
 h4   |  7
 h4   | 18
 h4   | 20
 h4   |  4
 h4   | 10
 h4   | 16
(20 rows)

When querying the data, the hash partition uses the same algorithm as the 
insertion, and filters out the table that does not need to be scanned.

postgres=# explain analyze select * from h where id = 1;
 QUERY PLAN 


 Append  (cost=0.00..41.88 rows=13 width=4) (actual time=0.020..0.023 rows=1 
loops=1)
   ->  Seq Scan on h3  (cost=0.00..41.88 rows=13 width=4) (actual 
time=0.013..0.016 rows=1 loops=1)
 Filter: (id = 1)
 Rows Removed by Filter: 3
 Planning time: 0.346 ms
 Execution time: 0.061 ms
(6 rows)

postgres=# explain analyze select * from h where id in (1,5);;
 QUERY PLAN 


 Append  (cost=0.00..83.75 rows=52 width=4) (actual time=0.016..0.028 rows=2 
loops=1)
   ->  Seq Scan on h1  (cost=0.00..41.88 rows=26 width=4) (actual 
time=0.015..0.018 rows=1 loops=1)
 Filter: (id = ANY ('{1,5}'::integer[]))
 Rows Removed by Filter: 6
   ->  Seq Scan on h3  (cost=0.00..41.88 rows=26 width=4) (actual 
time=0.005..0.007 rows=1 loops=1)
 Filter: (id = ANY ('{1,5}'::integer[]))
 Rows Removed by Filter: 3
 Planning time: 0.720 ms
 Execution time: 0.074 ms
(9 rows)

postgres=# explain analyze select * from h where id = 1 or id = 5;;
 QUERY PLAN 


 Append  (cost=0.00..96.50 rows=50 width=4) (actual time=0.017..0.078 rows=2 
loops=1)
   ->  Seq Scan on h1  (cost=0.00..48.25 rows=25 width=4) (actual 
time=0.015..0.019 rows=1 loops=1)
 Filter: ((id = 1) OR (id = 5))
 Rows Removed by Filter: 6
   ->  Seq Scan on h3  (cost=0.00..48.25 rows=25 width=4) (actual 
time=0.005..0.010 rows=1 loops=1)
 Filter: ((id = 1) OR (id = 5))
 Rows Removed by Filter: 3
 Planning time: 0.396 ms
 Execution time: 0.139 ms
(9 rows)

Can not detach / attach / drop partition table.

[HACKERS] Build failure on thrips

[Michael Meskes]

The bug fix I committed for fixing setlocale in ECPG test cases on Windows
seems to work nicely on all buildfarm animals except thrips. Could anyone with
access to a similar systems please check what's going on or send the
precompiled file that creates the compilation errors? 

Thanks.

Michael
-- 
Michael Meskes
Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org)
Meskes at (Debian|Postgresql) dot Org
Jabber: michael at xmpp dot meskes dot org
VfL Borussia! Força Barça! Go SF 49ers! Use Debian GNU/Linux, PostgreSQL


-- 
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] Not listed as committer

[Michael Meskes]

> Ha, that's interesting.
> 
> Should be fixed now, please try again. 

Works, thanks Magnus.

Michael
-- 
Michael Meskes
Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org)
Meskes at (Debian|Postgresql) dot Org
Jabber: michael at xmpp dot meskes dot org
VfL Borussia! Força Barça! SF 49ers! Use Debian GNU/Linux, PostgreSQL


-- 
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] ECPG: WHENEVER statement with DO CONTINUE action

[Michael Meskes]

> Given that it's Friday evening in Europe, I'm betting Michael is gone
> for the day.  In the interests of getting the buildfarm back to
> green,
> I'll see if I can fix this.

Correct, thanks Tom.

Michael
-- 
Michael Meskes
Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org)
Meskes at (Debian|Postgresql) dot Org
Jabber: michael at xmpp dot meskes dot org
VfL Borussia! Força Barça! SF 49ers! Use Debian GNU/Linux, PostgreSQL


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

[HACKERS] Re: [PATCH] Fix drop replication slot blocking instead of returning error

[Noah Misch]

On Thu, Aug 24, 2017 at 03:38:20PM +0200, Simone Gotti wrote:
> I noticed that in postgres 10beta3, calling pg_drop_replication_slot on an
> active slot will block until it's released instead of returning an error
> like
> done in pg 9.6. Since this is a change in the previous behavior and the docs
> wasn't changed I made a patch to restore the previous behavior.
> 
> Thanks,
> 
> Simone.
> 
> --
> 
> after git commit 9915de6c1cb calls to pg_drop_replication_slot or the
> replication protocol DROP_REPLICATION_SLOT command will block when a
> slot is in use instead of returning an error as before (as the doc
> states).
> 
> This patch will set nowait to true to restore the previous
> behavior.

[Action required within three days.  This is a generic notification.]

The above-described topic is currently a PostgreSQL 10 open item.  Álvaro,
since you committed the patch believed to have created it, you own this open
item.  If some other commit is more relevant or if this does not belong as a
v10 open item, please let us know.  Otherwise, please observe the policy on
open item ownership[1] and send a status update within three calendar days of
this message.  Include a date for your subsequent status update.  Testers may
discover new open items at any time, and I want to plan to get them all fixed
well in advance of shipping v10.  Consequently, I will appreciate your efforts
toward speedy resolution.  Thanks.

[1] 
https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.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] psql - add ability to test whether a variable exists

[Pavel Stehule]

2017-08-26 8:54 GMT+02:00 Fabien COELHO :

>
> Hello,
>
> As a follow-up to the \if patch by Corey Huinker, here is a proposal to
> allow testing whether a client-side variable exists in psql.
>
> The syntax is as ugly as the current :'var' and :"var" things, but ISTM
> that this is the only simple option to have a working SQL-compatible syntax
> with both client-side substitution and server side execution. See the
> second example below.
>

It is really ugly - the ? symbol is not used in pair usually - so it is
much more visible - it is bad readable.

Maybe some other syntax: :{fx xxx} .. where fx can be one from more
possible operators ? ! ...


>
>   -- client side use
>   psql> \set i 1
>   psql> \if :?i?
>   psql>   \echo 'i is defined'
>   psql> \endif


>
  -- use server-side in an SQL expression
>   psql> SELECT NOT :?VERSION_NUM? OR :'VERSION' <> VERSTION() AS bad_conf
> \gset
>




>   psql> \if :bad_conf \echo 'too bad...' \quit \endif
>
> The other option would be to have some special keyword syntax, say
> "defined var", but then it would have to be parsed client side, and how to
> do that in an SQL expression is unclear, and moreover it would not look
> right in an SQL expression. If it would look like a function call, say
> "defined('var')", it would potentially interact with existing server-side
> user-defined functions, which is pretty undesirable. Hence the :?...?
> proposal above which is limited to variable subsitution syntax.


should not be solved by introduction \ifdef ?



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

[HACKERS] psql - add ability to test whether a variable exists

[Fabien COELHO]

Hello,

As a follow-up to the \if patch by Corey Huinker, here is a proposal to 
allow testing whether a client-side variable exists in psql.


The syntax is as ugly as the current :'var' and :"var" things, but ISTM 
that this is the only simple option to have a working SQL-compatible 
syntax with both client-side substitution and server side execution. See 
the second example below.


  -- client side use
  psql> \set i 1
  psql> \if :?i?
  psql>   \echo 'i is defined'
  psql> \endif

  -- use server-side in an SQL expression
  psql> SELECT NOT :?VERSION_NUM? OR :'VERSION' <> VERSTION() AS bad_conf \gset
  psql> \if :bad_conf \echo 'too bad...' \quit \endif

The other option would be to have some special keyword syntax, say 
"defined var", but then it would have to be parsed client side, and how to
do that in an SQL expression is unclear, and moreover it would not look 
right in an SQL expression. If it would look like a function call, say 
"defined('var')", it would potentially interact with existing server-side 
user-defined functions, which is pretty undesirable. Hence the :?...? 
proposal above which is limited to variable subsitution syntax.


--
Fabien.

psql-ifdef-1.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] Variable substitution in psql backtick expansion

[Fabien COELHO]

Hello Tom,


...  I'm still not sure that there's any use case for the
string versions ("9.6.4" etc).



If somebody's doing comparisons, they probably want the numeric
version, but somebody might want to print the string version in an
error message e.g. \if  \echo this thing
doesn't work on :VERSION_NAME \quit \endif


OK, but if human-friendly display is the use-case then it ought to
duplicate what psql itself would print in, eg, the startup message about
server version mismatch.  The v4 patch does not, in particular it neglects
PQparameterStatus(pset.db, "server_version").  This would result in
printing, eg, "11.0" when the user would likely rather see "11devel".


I understand that you would prefer VERSION_NAME to show something like

  "11devel, server 9.6.4"

Instead of the current "11devel" when there is a client/server mismatch? I 
do not like it much. Note that the server version is already available as 
:SERVER_NAME/NUM.


Moreover I would like to point out that pre-existing :VERSION does not do 
such a thing. I was just extending it to have something more convenient 
and simple, hence the names.


Now they can be named :CLIENT_VERSION_NAME/NUM instead, as suggested by 
Robert, but that would be a little bit inconsistent with the existing 
VERSION...


Or maybe we could rename it CLIENT_VERSION as well, and make the ambiguous 
VERSION be the "11devel, server 9.6.4" thing?


In summary, my prefered option is to have:
  CLIENT_VERSION "PostgreSQL 11devel on ..."
  CLIENT_VERSION_NAME "11devel"
  CLIENT_VERSION_NUM 11
  SERVER_VERSION_NAME "9.6.4"
  SERVER_VERSION_NUM 090604
  maybe SERVER_VERSION for the long string?
  and VERSION as "11devel server 9.6.4" is no match, or just the short
  string if match, so that it is nearly upward compatible?

As always, the committer decides.

--
Fabien.


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