Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Fabien COELHO]

Welcome to v15, highlights:


Files "conditional.h" and "conditional.c" are missing from the patch.

Also, is there a particular reason why tap test have been removed?

--
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] tablesample with partitioned tables

[Amit Langote]

On 2017/02/23 0:54, David Fetter wrote:
> On Wed, Feb 22, 2017 at 04:51:46PM +0900, Amit Langote wrote:
>> Attached patch fixes an oversight that tablesample cannot be used with
>> partitioned tables:
>>
>> create table p (a int) partition by list (a);
>> select * from p tablesample bernoulli (50);
>> ERROR:  TABLESAMPLE clause can only be applied to tables and materialized
>> views
> 
> Thanks!
> 
> Should the error message change somehow to reflect that partitioned
> tables are included?  Is complete transparency of partitioned tables
> the goal, and reasonable in this context?

We avoid mentioning partitioned tables separately during most of the
errors caused by relkind checks.  I mentioned recently [1] that in most of
these sites such as this one, a table's being partitioned is not significant.

> Also, is there a good reason apart from tuits not to expand
> TABLESAMPLE to the rest of our SQL-visible relation structures?  I'm
> guessing this could have something to do with the volatility they
> might have, whether in views that call volatile functions or in
> foreign tables that might not make the right guarantees...

I wouldn't be able to say much about that, but I found an email from the
original discussion that occurred around development of this feature that
posed the same question.  There might be some answers there.

[1]
https://www.postgresql.org/message-id/854ad246-4dfa-5c68-19ad-867b6800f313%40lab.ntt.co.jp
[2] https://www.postgresql.org/message-id/5526D369.1070905%40gmx.net




-- 
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] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

On Wed, Feb 22, 2017 at 6:15 PM, Corey Huinker 
wrote:

> On Wed, Feb 22, 2017 at 5:59 PM, Tom Lane  wrote:
>
>> Ah, I see why *that* wants to know about it ... I think.  I suppose you're
>> arguing that variable expansion shouldn't be able to insert, say, an \else
>> in a non-active branch?  Maybe, but if it can insert an \else in an active
>> branch, then why not non-active too?  Seems a bit inconsistent.
>>
>
> The major reason was avoiding situations like what Daniel showed: where
> value of a variable that is meaningless/undefined in the current
> false-block context gets expanded anyway, and thus code inside a false
> block has effects outside of that block. Granted, his example was
> contrived. I'm open to removing that feature and seeing what breaks in the
> test cases.
>


Welcome to v15, highlights:
- all conditional data structure management moved to conditional.h and
conditional.c
- conditional state lives in mainloop.c and is passed to
HandleSlashCommands, exec_command and get_prompt as needed
- no more pset.active_branch, uses conditional_active(conditional_stack)
instead
- PsqlScanState no longer has branching state
- Implements the %R '@' prompt on false branches.
- Variable expansion is never suppressed even in false blocks, regression
test edited to reflect this.
- ConditionalStack could morph into PsqlFileState without too much work.
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index ae58708..dac8e37 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -2035,6 +2035,79 @@ hello 10
 
   
 
+  
+\if expr
+\elif expr
+\else
+\endif
+
+
+This group of commands implements nestable conditional blocks, like
+this:
+
+
+-- set ON_ERROR_STOP in case the variables are not valid boolean expressions
+\set ON_ERROR_STOP on
+SELECT
+EXISTS(SELECT 1 FROM customer WHERE customer_id = 123) as is_customer,
+EXISTS(SELECT 1 FROM employee WHERE employee_id = 456) as is_employee
+\gset
+\if :is_customer
+SELECT * FROM customer WHERE customer_id = 123;
+\elif :is_employee
+\echo 'is not a customer but is an employee'
+SELECT * FROM employee WHERE employee_id = 456;
+\else
+\if yes
+\echo 'not a customer or employee'
+\else
+\echo 'this should never print'
+\endif
+\endif
+
+
+Conditional blocks must begin with a \if and end
+with an \endif, and the pairs must be found in
+the same source file. If an EOF is reached on the main file or an
+\include-ed file before all local
+\if-\endif are matched, then
+psql will raise an error.
+
+
+A conditional block can have any number of 
+\elif clauses, which may optionally be followed by a
+single \else clause.
+
+
+The \if and \elif commands
+read the rest of the line and evaluate it as a boolean expression.
+Currently, expressions are limited to a single unquoted string
+which are evaluated like other on/off options, so the valid values
+are any unambiguous case insensitive matches for one of:
+true, false, 
1,
+0, on, off,
+yes, no.  So 
+t, T, and tR
+will all match true.
+
+Expressions that do not properly evaluate to true or false will
+generate an error and cause the \if or
+\elif command to fail.  Because that behavior may
+change branching context in undesirable ways (executing code which
+was intended to be skipped, causing \elif,
+\else, and \endif commands to
+pair with the wrong \if, etc), it is
+recommended that scripts which use conditionals also set
+ON_ERROR_STOP.
+
+
+Lines within false branches are not evaluated in any way: queries are
+not sent to the server, non-conditional commands are not evaluated but
+bluntly ignored, nested if-expressions in such branches are also not
+evaluated but are tallied to check for proper nesting.
+
+
+  
 
   
 \ir or \include_relative 
filename
diff --git a/src/bin/psql/Makefile b/src/bin/psql/Makefile
index c53733f..1492b66 100644
--- a/src/bin/psql/Makefile
+++ b/src/bin/psql/Makefile
@@ -21,7 +21,7 @@ REFDOCDIR= $(top_srcdir)/doc/src/sgml/ref
 override CPPFLAGS := -I. -I$(srcdir) -I$(libpq_srcdir) $(CPPFLAGS)
 LDFLAGS += -L$(top_builddir)/src/fe_utils -lpgfeutils -lpq
 
-OBJS=  command.o common.o help.o input.o stringutils.o mainloop.o copy.o \
+OBJS=  command.o common.o conditional.o help.o input.o stringutils.o 
mainloop.o copy.o \
startup.o prompt.o variables.o large_obj.o describe.o \
crosstabview.o tab-complete.o \
sql_help.o psqlscanslash.o \
@@ -61,8 +61,16 @@ uninstall:
 
 clean distclean:

Re: [HACKERS] Partitioned tables and relfilenode

[Amit Langote]

Thanks for the review.

On 2017/02/22 21:58, Ashutosh Bapat wrote:
>>> Also we should add tests to make sure the scans on partitioned tables
>>> without any partitions do not get into problems. PFA patch which adds
>>> those tests.
>>
>> I added the test case you suggest, but kept just the first one.
> 
> The second one was testing multi-level partitioning case, which
> deserves a testcase by its own.

Ah, okay.  Added it back.

> Some more comments
> 
> The comment below seems too verbose. All it can say is "A partitioned table
> without any partitions results in a dummy relation." I don't think we need to
> be explicit about rte->inh. But it's more of personal preference. We will 
> leave
> it to the committer, if you don't agree.
> +   /*
> +* An empty partitioned table, i.e., one without any leaf
> +* partitions, as signaled by rte->inh being set to false
> +* by the prep phase (see expand_inherited_rtentry).
> +*/

It does seem poorly worded.  How about:

/*
 * A partitioned table without leaf partitions is marked
 * as a dummy rel.
 */

> We don't need this comment as well. Rather than repeating what's been said at
> the top of the function, it should just refer to it like "nominal relation has
> been set above for partitioned tables. For other parent relations, we'll use
> the first child ...".
> +*
> +* If the parent is a partitioned table, we do not have a separate RTE
> +* representing it as a member of the inheritance set, because we do
> +* not create a scan plan for it.  As mentioned at the top of this
> +* function, we make the parent RTE itself the nominal relation.
>  */

Rewrote that comment block as:

 *
 * If the parent is a partitioned table, we already set the nominal
 * relation.
 */


> Following condition is not very readable. It's not evident that it's of the
> form (A && B) || C, at a glance it looks like it's A && (B || C).
> +   if ((rte->relkind != RELKIND_PARTITIONED_TABLE &&
> +list_length(appinfos) < 2) || list_length(appinfos) < 1)
> 
> Instead you may rearrage it as
> min_child_rels = (rte->relkind == RELKIND_PARTITIONED_TABLE ? 1 : 2);
> if (list_length(appinfos) < min_child_rels)

OK, done that way.

Thanks,
Amit
>From ed28c10fa4c4ae30beb1dbc621b4d38d31f718e1 Mon Sep 17 00:00:00 2001
From: amit 
Date: Mon, 6 Feb 2017 18:03:28 +0900
Subject: [PATCH 1/3] Partitioned tables are empty themselves

So, there is not much point in trying to do things to them that need
accessing files (a later commit will make it so that there is no file
at all to access.)  Things that needed attention are: vacuum, analyze,
truncate, ATRewriteTables.
---
 doc/src/sgml/ddl.sgml   |  7 ++--
 src/backend/commands/analyze.c  | 39 ++--
 src/backend/commands/tablecmds.c| 15 ++--
 src/backend/commands/vacuum.c   | 71 +
 src/backend/postmaster/autovacuum.c | 20 +++
 5 files changed, 123 insertions(+), 29 deletions(-)

diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml
index ef0f7cf727..c10d312139 100644
--- a/doc/src/sgml/ddl.sgml
+++ b/doc/src/sgml/ddl.sgml
@@ -3840,8 +3840,11 @@ UNION ALL SELECT * FROM measurement_y2008m01;
 
 ANALYZE measurement;
 
-  will only process the master table.  This is true even for partitioned
-  tables.
+  will only process the master table.
+ 
+
+ 
+  This does not apply to partitioned tables though.
  
 
 
diff --git a/src/backend/commands/analyze.c b/src/backend/commands/analyze.c
index ed3acb1673..12cd0110b0 100644
--- a/src/backend/commands/analyze.c
+++ b/src/backend/commands/analyze.c
@@ -201,8 +201,7 @@ analyze_rel(Oid relid, RangeVar *relation, int options,
 	 * locked the relation.
 	 */
 	if (onerel->rd_rel->relkind == RELKIND_RELATION ||
-		onerel->rd_rel->relkind == RELKIND_MATVIEW ||
-		onerel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
+		onerel->rd_rel->relkind == RELKIND_MATVIEW)
 	{
 		/* Regular table, so we'll use the regular row acquisition function */
 		acquirefunc = acquire_sample_rows;
@@ -234,6 +233,12 @@ analyze_rel(Oid relid, RangeVar *relation, int options,
 			return;
 		}
 	}
+	else if (onerel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
+	{
+		/*
+		 * For partitioned tables, we want to do the recursive ANALYZE below.
+		 */
+	}
 	else
 	{
 		/* No need for a WARNING if we already complained during VACUUM */
@@ -253,13 +258,15 @@ analyze_rel(Oid relid, RangeVar *relation, int options,
 	LWLockRelease(ProcArrayLock);
 
 	/*
-	 * Do the normal non-recursive ANALYZE.
+	 * Do the normal non-recursive ANALYZE, non-partitioned relations.
 	 */
-	do_analyze_rel(onerel, options, params, va_cols, acquirefunc, relpages,
-   false, in_outer_xact, elevel);
+	if 

Re: [HACKERS] Measuring replay lag

[Thomas Munro]

On Thu, Feb 23, 2017 at 11:52 AM, Thomas Munro
 wrote:
> The overall graph looks pretty similar, but it is more likely to short
> hiccups caused by occasional slow WAL fsyncs in walreceiver.   See the

I meant to write "more likely to *miss* short hiccups".

-- 
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] Enabling parallelism for queries coming from SQL or other PL functions

[Rafia Sabih]

On Wed, Feb 22, 2017 at 10:22 PM, Dilip Kumar  wrote:
>
> Some initial comments.
>
> --
> if (numberTuples || dest->mydest == DestIntoRel)
> use_parallel_mode = false;
>
> if (use_parallel_mode)
> EnterParallelMode();
> + else if (estate->es_plannedstmt->parallelModeNeeded &&
> + (dest->mydest == DestSPI || dest->mydest == DestSQLFunction))
> + {
> + use_parallel_mode = true;
> + EnterParallelMode();
> + }
>
> I think we can simplify this, can we replace above code with something
> like this?
>
> if (dest->mydest == DestIntoRel ||
> numberTuples && (dest->mydest != DestSPI || dest->mydest !=
> DestSQLFunction))
> use_parallel_mode = false;

Yes, it can be simplified to
if (dest->mydest == DestIntoRel || (numberTuples && (dest->mydest !=
DestSPI && dest->mydest ! DestSQLFunction)))
Thanks.
>
> -
>
> + {
> + /* Allow parallelism if the function is not trigger type. */
> + if (estate->func->fn_is_trigger == PLPGSQL_NOT_TRIGGER)
> + exec_res = SPI_execute(querystr, estate->readonly_func,
> CURSOR_OPT_PARALLEL_OK);
> + else
> + exec_res = SPI_execute(querystr, estate->readonly_func, 0);
> + }
>
> The last parameter of SPI_execute is tuple count, not cursorOption,
> you need to fix this. Also, this is crossing the 80 line boundary.
>
Oops, corrected.

> ---
> Any specific reason for not changing SPI_execute_with_args, EXECUTE
> with USING will take this path.
>
Fixed.


-- 
Regards,
Rafia Sabih
EnterpriseDB: http://www.enterprisedb.com/


pl_parallel_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 pg_disable_checksums() and supporting infrastructure

[Jim Nasby]

On 2/20/17 11:22 AM, David Christensen wrote:

- If an entire cluster is going to be considered as checksummed, then even 
databases that don't allow connections would need to get enabled.

Yeah, the workaround for now would be to require “datallowconn" to be set to 
’t' for all databases before proceeding, unless there’s a way to connect to those 
databases internally that bypasses that check.  Open to ideas, though for a first 
pass seems like the “datallowconn” approach is the least amount of work.


The problem with ignoring datallowconn is any database where that's 
false is fair game for CREATE DATABASE. I think just enforcing that 
everything's connectable is good enough for now.



I like the idea of revalidation, but I'd suggest leaving that off of the first 
pass.

Yeah, agreed.


It might be easier on a first pass to look at supporting per-database checksums 
(in this case, essentially treating shared catalogs as their own database). All 
normal backends do per-database stuff (such as setting current_database) during 
startup anyway. That doesn't really help for things like recovery and 
replication though. :/ And there's still the question of SLRUs (or are those 
not checksum'd today??).

So you’re suggesting that the data_checksums GUC get set per-database context, 
so once it’s fully enabled in the specific database it treats it as in 
enforcing state, even if the rest of the cluster hasn’t completed?  Hmm, might 
think on that a bit, but it seems pretty straightforward.


Something like that, yeah.


What issues do you see affecting replication and recovery specifically (other 
than the entire cluster not being complete)?  Since the checksum changes are 
WAL logged, seems you be no worse the wear on a standby if you had to change 
things.


I'm specifically worried about the entire cluster not being complete. 
That makes it harder for replicas to know what blocks they can and can't 
verify the checksum on.


That *might* still be simpler than trying to handle converting the 
entire cluster in one shot. If it's not simpler I certainly wouldn't do 
it right now.



BTW, it occurs to me that this is related to the problem we have with trying to make changes that 
break page binary compatibility. If we had a method for handling that it would probably be useful 
for enabling checksums as well. You'd essentially treat an un-checksum'd page as if it was an 
"old page version". The biggest problem there is dealing with the potential that the new 
page needs to be larger than the old one was, but maybe there's some useful progress to be had in 
this area before tackling the "page too small" problem.

I agree it’s very similar; my issue is I don’t want to have to postpone 
handling a specific case for some future infrastructure.


Yeah, I was just mentioning it.
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)


--
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] Partitioned tables and relfilenode

[Ashutosh Bapat]

On Thu, Feb 23, 2017 at 11:19 AM, Amit Langote
 wrote:
> Thanks for the review.
>
> On 2017/02/22 21:58, Ashutosh Bapat wrote:
 Also we should add tests to make sure the scans on partitioned tables
 without any partitions do not get into problems. PFA patch which adds
 those tests.
>>>
>>> I added the test case you suggest, but kept just the first one.
>>
>> The second one was testing multi-level partitioning case, which
>> deserves a testcase by its own.
>
> Ah, okay.  Added it back.

Thanks.

>
>> Some more comments
>>
>> The comment below seems too verbose. All it can say is "A partitioned table
>> without any partitions results in a dummy relation." I don't think we need to
>> be explicit about rte->inh. But it's more of personal preference. We will 
>> leave
>> it to the committer, if you don't agree.
>> +   /*
>> +* An empty partitioned table, i.e., one without any leaf
>> +* partitions, as signaled by rte->inh being set to false
>> +* by the prep phase (see expand_inherited_rtentry).
>> +*/
>
> It does seem poorly worded.  How about:
>
> /*
>  * A partitioned table without leaf partitions is marked
>  * as a dummy rel.
>  */
>
>> We don't need this comment as well. Rather than repeating what's been said at
>> the top of the function, it should just refer to it like "nominal relation 
>> has
>> been set above for partitioned tables. For other parent relations, we'll use
>> the first child ...".
>> +*
>> +* If the parent is a partitioned table, we do not have a separate 
>> RTE
>> +* representing it as a member of the inheritance set, because we do
>> +* not create a scan plan for it.  As mentioned at the top of this
>> +* function, we make the parent RTE itself the nominal relation.
>>  */
>
> Rewrote that comment block as:
>
>  *
>  * If the parent is a partitioned table, we already set the nominal
>  * relation.
>  */
>

I reworded those comments a bit and corrected grammar. Please check in
the attached patch.

>
>> Following condition is not very readable. It's not evident that it's of the
>> form (A && B) || C, at a glance it looks like it's A && (B || C).
>> +   if ((rte->relkind != RELKIND_PARTITIONED_TABLE &&
>> +list_length(appinfos) < 2) || list_length(appinfos) < 1)
>>
>> Instead you may rearrage it as
>> min_child_rels = (rte->relkind == RELKIND_PARTITIONED_TABLE ? 1 : 2);
>> if (list_length(appinfos) < min_child_rels)
>
> OK, done that way.

On a second thought, I added a boolean to check if there were any
children created and then reset rte->inh based on that value. That's
better than relying on appinfos length now.

Let me know what you think.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company


0003-Avoid-creating-scan-nodes-for-partitioned-tables_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] tablesample with partitioned tables

[David Fetter]

On Wed, Feb 22, 2017 at 04:51:46PM +0900, Amit Langote wrote:
> Attached patch fixes an oversight that tablesample cannot be used with
> partitioned tables:
> 
> create table p (a int) partition by list (a);
> select * from p tablesample bernoulli (50);
> ERROR:  TABLESAMPLE clause can only be applied to tables and materialized
> views

Thanks!

Should the error message change somehow to reflect that partitioned
tables are included?  Is complete transparency of partitioned tables
the goal, and reasonable in this context?

Also, is there a good reason apart from tuits not to expand
TABLESAMPLE to the rest of our SQL-visible relation structures?  I'm
guessing this could have something to do with the volatility they
might have, whether in views that call volatile functions or in
foreign tables that might not make the right guarantees...

Best,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter  XMPP: david(dot)fetter(at)gmail(dot)com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate


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

Re: [HACKERS] Allow pg_dumpall to work without pg_authid

[David Fetter]

On Wed, Feb 22, 2017 at 06:33:10PM +1100, Robins Tharakan wrote:
> Stephen,
> 
> On 20 February 2017 at 08:50, Stephen Frost  wrote:
> 
> > The other changes to use pg_roles instead of pg_authid when rolpassword
> > isn't being used look like they should just be changed to use pg_roles
> > instead of using one or the other.  That should be an independent patch
> > from the one which adds the option we are discussing.
> >
> 
> Sure. Attached are 2 patches, of which 1 patch just replaces
> ​pg_authid with pg_roles in pg_dumpall. The only exceptions
> there are buildShSecLabels() &
> pg_catalog.binary_upgrade_set_next_pg_authid_oid() which I thought
> should still use pg_authid.

Thanks for doing this!

That pg_dumpall didn't work with RDS Postgres (and possibly others)
was a pretty large wart.

In future, could you please leave patches uncompressed so they're
easier to see in the archives?

Best,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter  XMPP: david(dot)fetter(at)gmail(dot)com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate


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