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

[Corey Huinker]

>
> Maybe this can be a discussed in a follow-up patch and Corey should
> proceed to finalize the if patch?


In the event that we can leave prompting to a later patch, here are the v12
highlights:
- created conditional.h and conditional.c which contain the functions with
stack-ish push/pop/peek/poke names
- now all non-test, non-doc changes are in src/bin/psql
- moved conditional stack out of scan_state, stack state maintained by
mainloop.c/startup.c, passed to HandleSlashCommands
- documentation encourages the user to employ ON_ERROR_STOP when using
conditionals
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index ae58708..bdef6e9 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -2035,6 +2035,77 @@ hello 10
 
   
 
+  
+\if expr
+\elif expr
+\else
+\endif
+
+
+This group of commands implements nestable conditional blocks, like
+this:
+
+
+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:
rm -f psql$(X) $(OBJS) lex.backup
+   rm -rf tmp_check
 
 # files removed here are supposed to be in the distribution tarball,
 # so do not clean them in the clean/distclean rules
 maintainer-clean: distclean
rm -f sql_help.h sql_help.c psqlscanslash.c
+
+
+check:
+   $(prove_check)
+
+installcheck:
+   $(prove_installcheck)
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index f17f610..fded2bc 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -38,6 +38,7 @@
 #include "fe_utils/string_utils.h"
 
 #include "common.h"
+#include "conditional.h"
 #include "copy.h"
 #include "crosstabview.h"
 #include "describe.h"
@@ -62,6 +63,7 @@ typedef enum EditableObjectType
 /* functions for use in this file */
 static backslashResult exec_command(const char *cmd,
 PsqlScanState scan_state,
+ConditionalStack cstack,
 PQExpBuffer que

Re: [HACKERS] GUC for cleanup indexes threshold.

[Masahiko Sawada]

On Fri, Feb 10, 2017 at 8:01 PM, Kuntal Ghosh
 wrote:
> On Wed, Jan 4, 2017 at 1:51 PM, Masahiko Sawada  wrote:
>
>> Attached patch introduces new GUC parameter parameter
>> vacuum_cleanup_index_scale_factor which specifies the fraction of the
>> table pages containing dead tuple needed to trigger a cleaning up
>> indexes. The default is 0.0, which means that the cleanup index is not
>> invoked if no update on table. In other word, if table is completely
>> frozen then lazy vacuum can skip the index scans as well. Increasing
>> this value could reduce total time of lazy vacuum but the statistics
>> and the free space map of index are not updated.
>>
> I was looking into your patch and trying to understand how the
> following piece of code works.

Thank you for looking at this patch!

> +if (vacuumed_pages > cleanupidx_thresh)
> +{
> +for (i = 0; i < nindexes; i++)
> +lazy_cleanup_index(Irel[i], indstats[i], vacrelstats);
> +}
> So, you are skipping deletion of index entries if it does not reach
> the clean-up index threshold. But, you are removing all dead tuples
> from the heap pointed by the same index. Hence, index will contain
> entries with invalid references.

I think no. Before calling lazy_cleanup_index, all garbage on heap and
index should have been reclaimed by lazy_vacuum_heap and
lazy_vacuum_index.

> +This parameter can only be set anywhere.
> Oxymoron. :-)
>

Will fix it.

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

[HACKERS] AT detach partition is broken

[Amit Langote]

I noticed that running ALTER TABLE table_name DETACH PARTITION crashes, if
table_name is not a partitioned table.  That's because of an  Assert in
ATExecDetachPartition().  We really should error out much sooner in this
case, IOW during transformAlterTableStmt(), as is done in the case of
ATTACH PARTITION.

Attached patch fixes that.

Thanks,
Amit
>From d74189febc751302f13009d69bebd763c8a23f58 Mon Sep 17 00:00:00 2001
From: amit 
Date: Mon, 13 Feb 2017 16:10:17 +0900
Subject: [PATCH] Unbreak ALTER TABLE DETACH PARTITION

DETACH PARTITION should throw error much sooner if the specified
(parent) table is not a partitioned table.  In case of ATTACH
PARTITION that's done during transformAlterTableStmt.  Do the same
for DETACH PARTITION.  Instead of inventing a transformDetachPartition,
rename the existing transformAttachPartition to transformPartitionCmd
and have it serve both cases.
---
 src/backend/parser/parse_utilcmd.c| 29 +++--
 src/test/regress/expected/alter_table.out |  5 +
 src/test/regress/sql/alter_table.sql  |  5 +
 3 files changed, 29 insertions(+), 10 deletions(-)

diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c
index 0f78abaae2..efbd205b63 100644
--- a/src/backend/parser/parse_utilcmd.c
+++ b/src/backend/parser/parse_utilcmd.c
@@ -133,7 +133,7 @@ static void transformConstraintAttrs(CreateStmtContext *cxt,
 		 List *constraintList);
 static void transformColumnType(CreateStmtContext *cxt, ColumnDef *column);
 static void setSchemaName(char *context_schema, char **stmt_schema_name);
-static void transformAttachPartition(CreateStmtContext *cxt, PartitionCmd *cmd);
+static void transformPartitionCmd(CreateStmtContext *cxt, PartitionCmd *cmd);
 
 
 /*
@@ -2654,13 +2654,18 @@ transformAlterTableStmt(Oid relid, AlterTableStmt *stmt,
 }
 
 			case AT_AttachPartition:
+			case AT_DetachPartition:
 {
 	PartitionCmd *partcmd = (PartitionCmd *) cmd->def;
 
-	transformAttachPartition(&cxt, partcmd);
+	transformPartitionCmd(&cxt, partcmd);
 
-	/* assign transformed values */
-	partcmd->bound = cxt.partbound;
+	/*
+	 * Assign transformed value of the partition bound, if
+	 * any.
+	 */
+	if (cxt.partbound != NULL)
+		partcmd->bound = cxt.partbound;
 }
 
 newcmds = lappend(newcmds, cmd);
@@ -3032,11 +3037,14 @@ setSchemaName(char *context_schema, char **stmt_schema_name)
 }
 
 /*
- * transformAttachPartition
- *		Analyze ATTACH PARTITION ... FOR VALUES ...
+ * transformPartitionCmd
+ *		Analyze the ATTACH/DETACH PARTITION command
+ *
+ * In case of the ATTACH PARTITION command, cxt->partbound is set to the
+ * transformed value of cmd->bound.
  */
 static void
-transformAttachPartition(CreateStmtContext *cxt, PartitionCmd *cmd)
+transformPartitionCmd(CreateStmtContext *cxt, PartitionCmd *cmd)
 {
 	Relation	parentRel = cxt->rel;
 
@@ -3050,10 +3058,11 @@ transformAttachPartition(CreateStmtContext *cxt, PartitionCmd *cmd)
  errmsg("\"%s\" is not partitioned",
 		RelationGetRelationName(parentRel;
 
-	/* transform the values */
+	/* transform the partition bound, if any */
 	Assert(RelationGetPartitionKey(parentRel) != NULL);
-	cxt->partbound = transformPartitionBound(cxt->pstate, parentRel,
-			 cmd->bound);
+	if (cmd->bound != NULL)
+		cxt->partbound = transformPartitionBound(cxt->pstate, parentRel,
+ cmd->bound);
 }
 
 /*
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index d8e7b61294..33f820a245 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -3255,6 +3255,11 @@ DETAIL:  "list_parted2" is already a child of "list_parted2".
 --
 -- DETACH PARTITION
 --
+-- check that the table is partitioned at all
+CREATE TABLE regular_table (a int);
+ALTER TABLE regular_table DETACH PARTITION any_name;
+ERROR:  "regular_table" is not partitioned
+DROP TABLE regular_table;
 -- check that the partition being detached exists at all
 ALTER TABLE list_parted2 DETACH PARTITION part_4;
 ERROR:  relation "part_4" does not exist
diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql
index 1f551ec53c..a2baf0f2a0 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -2134,6 +2134,11 @@ ALTER TABLE list_parted2 ATTACH PARTITION list_parted2 FOR VALUES IN (0);
 -- DETACH PARTITION
 --
 
+-- check that the table is partitioned at all
+CREATE TABLE regular_table (a int);
+ALTER TABLE regular_table DETACH PARTITION any_name;
+DROP TABLE regular_table;
+
 -- check that the partition being detached exists at all
 ALTER TABLE list_parted2 DETACH PARTITION part_4;
 
-- 
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

Re: [HACKERS] DROP SUBSCRIPTION and ROLLBACK

[Masahiko Sawada]

On Sat, Feb 11, 2017 at 8:18 PM, Petr Jelinek
 wrote:
> On 10/02/17 19:55, Masahiko Sawada wrote:
>> On Thu, Feb 9, 2017 at 12:44 AM, Petr Jelinek
>>  wrote:
>>> On 08/02/17 07:40, Masahiko Sawada wrote:
 On Wed, Feb 8, 2017 at 9:01 AM, Michael Paquier
  wrote:
> On Wed, Feb 8, 2017 at 1:30 AM, Fujii Masao  wrote:
>> On Wed, Feb 8, 2017 at 12:26 AM, Petr Jelinek
>>  wrote:
>>> For example what happens if apply crashes during the DROP
>>> SUBSCRIPTION/COMMIT and is not started because the delete from catalog
>>> is now visible so the subscription is no longer there?
>>
>> Another idea is to treat DROP SUBSCRIPTION in the same way as VACUUM, 
>> i.e.,
>> make it emit an error if it's executed within user's transaction block.
>
> It seems to me that this is exactly Petr's point: using
> PreventTransactionChain() to prevent things to happen.

 Agreed. It's better to prevent to be executed inside user transaction
 block. And I understood there is too many failure scenarios we need to
 handle.

>> Also DROP SUBSCRIPTION should call CommitTransactionCommand() just
>> after removing the entry from pg_subscription, then connect to the 
>> publisher
>> and remove the replication slot.
>
> For consistency that may be important.

 Agreed.

 Attached patch, please give me feedback.

>>>
>>> This looks good (and similar to what initial patch had btw). Works fine
>>> for me as well.
>>>
>>> Remaining issue is, what to do about CREATE SUBSCRIPTION then, there are
>>> similar failure scenarios there, should we prevent it from running
>>> inside transaction as well?
>>>
>>
>> Hmm,  after thought I suspect current discussing approach. For
>> example, please image the case where CRAETE SUBSCRIPTION creates
>> subscription successfully but fails to create replication slot for
>> whatever reason, and then DROP SUBSCRIPTION drops the subscription but
>> dropping replication slot is failed. In such case, CREAET SUBSCRIPTION
>> and DROP SUBSCRIPTION return ERROR but the subscription is created and
>> dropped successfully. I think that this behaviour confuse the user.
>>
>> I think we should just prevent calling DROP SUBSCRIPTION in user's
>> transaction block. Or I guess that it could be better to separate the
>> starting/stopping logical replication from subscription management.
>>
>
> We need to stop the replication worker(s) in order to be able to drop
> the slot. There is no such issue with startup of the worker as that one
> is launched by launcher after the transaction has committed.
>
> IMO best option is to just don't allow DROP/CREATE SUBSCRIPTION inside a
> transaction block and don't do any commits inside of those (so that
> there are no rollbacks, which solves your initial issue I believe). That
> way failure to create/drop slot will result in subscription not being
> created/dropped which is what we want.
>

I basically agree this option, but why do we need to change CREATE
SUBSCRIPTION as well?

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] Reporting xmin from VACUUMs

[Masahiko Sawada]

On Sat, Feb 11, 2017 at 7:00 AM, Robert Haas  wrote:
> On Fri, Feb 10, 2017 at 3:30 AM, Simon Riggs  wrote:
>> We've added xmin info to pg_stat_activity and pg_stat_replication, but
>> VACUUM doesn't yet report which xmin value it used when it ran.
>>
>> Small patch to add this info to VACUUM output.

+1

> It seems sensible to me to do something like this.  We already report
> a lot of other fine details, so what's one more?  And it could be
> useful.

Also, I've been proposing to report the number of skipped the frozen
pages even in manual vacuum verbose log.

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] contrib modules and relkind check

[Michael Paquier]

On Fri, Feb 10, 2017 at 4:29 PM, Amit Langote
 wrote:
> On 2017/02/10 14:32, Michael Paquier wrote:
>> On Thu, Feb 9, 2017 at 7:21 AM, Corey Huinker  
>> wrote:
>
> Thanks Corey and Michael for the reviews!
>
>>> 1. should have these tests named in core functions, like maybe:
>>>
>>> relation_has_visibility(Relation)
>>> relation_has_storage(Relation)
>>> and/or corresponding void functions check_relation_has_visibility() and
>>> check_relation_has_storage() which would raise the appropriate error message
>>> when the boolean test fails.
>>> Then again, this might be overkill since we don't add new relkinds very
>>> often.
>>
>> The visibility checks are localized in pg_visibility.c and the storage
>> checks in pgstatindex.c, so yes we could have macros in those files.
>> Or even better: just have a sanity check routine as the error messages
>> are the same everywhere.
>
> If I understand correctly what's being proposed here, tablecmds.c has
> something called ATWrongRelkindError() which sounds (kind of) similar.
> It's called by ATSimplePermissions() whenever it finds out that relkind of
> the table specified in a given ALTER TABLE command is not in the "allowed
> relkind targets" for the command.  Different ALTER TABLE commands call
> ATSimplePermissions() with an argument that specifies the "allowed relkind
> targets" for each command.   I'm not saying that it should be used
> elsewhere, but if we do invent a new centralized routine for relkind
> checks, it would look similar.

You did not get completely what I wrote upthread. This check is
repeated 3 times in pg_visibility.c:
+   /* Other relkinds don't have visibility info */
+   if (rel->rd_rel->relkind != RELKIND_RELATION &&
+   rel->rd_rel->relkind != RELKIND_MATVIEW &&
+   rel->rd_rel->relkind != RELKIND_TOASTVALUE)
+   ereport(ERROR,
+   (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+errmsg("\"%s\" is not a table, materialized view, or
TOAST table",
+   RelationGetRelationName(rel;

And this one is present 4 times in pgstatindex.c:
+   /* only the following relkinds have storage */
+   if (rel->rd_rel->relkind != RELKIND_RELATION &&
+   rel->rd_rel->relkind != RELKIND_INDEX &&
+   rel->rd_rel->relkind != RELKIND_MATVIEW &&
+   rel->rd_rel->relkind != RELKIND_SEQUENCE &&
+   rel->rd_rel->relkind != RELKIND_TOASTVALUE)
+   ereport(ERROR,
+   (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+errmsg("\"%s\" is not a table, index, materialized
view, sequence, or TOAST table",
+   RelationGetRelationName(rel;

So the suggestion is simply to encapsulate such blocks into their own
function like check_relation_relkind, each one locally in their
respective contrib's file. And each function includes the error
message, which should use ERRCODE_WRONG_OBJECT_TYPE as errcode by the
way.

>>> 2. Is it better stylistically to have an AND-ed list of != tests, or a
>>> negated list of OR-ed equality tests, and should the one style be changed to
>>> conform to the other?
>
> I changed the patch so that all newly added checks use an AND-ed list of
> != tests.
>
>>> No new regression tests. I think we should create a dummy partitioned table
>>> for each contrib and show that it fails.
>>
>> Yep, good point. That's easy enough to add.
>
> I added tests with a partitioned table to pageinspect's page.sql and
> pgstattuple.sql.  I don't see any tests for pg_visibility module, maybe I
> should add?

Checking those error code paths would be nice. It would be nice as
well that the relation types that are expected to be supported and
unsupported are checked. For pageinspect the check range is correct.
There are some relkinds missing in pgstatindex though.

> Although, I felt a bit uncomfortable the way the error message looks, for
> example:
>
> + -- check that using any of these functions with a partitioned table
> would fail
> + create table test_partitioned (a int) partition by range (a);
> + select pg_relpages('test_partitioned');
> + ERROR:  "test_partitioned" is not a table, index, materialized view,
> sequence, or TOAST table

Would it be a problem to mention this relkind as just "partitioned
table" in the error message.

> test_partitioned IS a table but just the kind without storage.  This is
> not the only place however where we do not separate the check for
> partitioned tables from other unsupported relkinds in respective contexts.
>  Not sure if that would be a better idea.

Well, perhaps I am playing with the words in my last paragraph, but
the documentation clearly states that any relation defined with
PARTITION BY is a "partitioned table".
-- 
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] possibility to specify template database for pg_regress

[Michael Paquier]

On Sat, Feb 11, 2017 at 3:03 PM, Pavel Stehule  wrote:
> here is new update - check is done before any creating

It may be better to do any checks before dropping existing databases
as well... It would be as well just simpler to complain with a single
error message like "database and template list lengths do not match".
-- 
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] REINDEX CONCURRENTLY 2.0

[Michael Paquier]

On Sun, Feb 12, 2017 at 6:44 AM, Andreas Karlsson  wrote:
> On 02/02/2015 03:10 PM, Andres Freund wrote:
>> I think if we should instead just use the new index, repoint the
>> dependencies onto the new oid, and then afterwards, when dropping,
>> rename the new index one onto the old one. That means the oid of the
>> index will change and some less than pretty grovelling around
>> dependencies, but it still seems preferrable to what we're discussing
>> here otherwise.
>
> I think that sounds like a good plan. The oid change does not seem like a
> too big deal to me, especially since that is what users will get now too. Do
> you still think this is the right way to solve this?

That hurts mainly system indexes. Perhaps users with broken system
indexes are not going to care about concurrency anyway. Thinking now
about it I don't see how that would not work, but I did not think
deeply about this problem lately.

> I have attached my work in progress patch which implements and is very
> heavily based on Michael's previous work. There are some things left to do
> but I think I should have a patch ready for the next commitfest if people
> still like this type of solution.

Cool to see a rebase of this patch. It's been a long time...

> I also changed index_set_state_flags() to be transactional since I wanted
> the old index to become invalid at exactly the same time as the new becomes
> valid. From reviewing the code that seems like a safe change.
>
> A couple of bike shedding questions:
>
> - Is the syntax "REINDEX  CONCUURENTLY " ok?

Yeah, that's fine. At least that's what has been concluded in previous threads.

> - What should we do with REINDEX DATABASE CONCURRENTLY and the system
> catalog? I so not think we can reindex the system catalog concurrently
> safely, so what should REINDEX DATABASE do with the catalog indexes? Skip
> them, reindex them while taking locks, or just error out?

System indexes cannot have their OIDs changed as they are used in
syscache lookups. So just logging a warning looks fine to me, and the
price to pay to avoid taking an exclusive lock even for a short amount
of time.

> - What level of information should be output in VERBOSE mode?

Er, something like that as well, no?
DETAIL:  CPU: user: 0.00 s, system: 0.00 s, elapsed: 0.00 s.

> What remains to be implemented:
> - Support for exclusion constraints
> - Look more into how I handle constraints (currently the temporary index too
> seems to have the PRIMARY KEY flag)
> - Support for the VERBOSE flag
> - More testing to catch bugs

This is a crasher:
create table aa (a int primary key);
reindex (verbose) schema concurrently public ;

For invalid indexes sometimes snapshots are still active (after
issuing the previous crash for example):
=# reindex (verbose) table concurrently aa;
WARNING:  XX002: cannot reindex concurrently invalid index
"public.aa_pkey_cct", skipping
LOCATION:  ReindexRelationConcurrently, indexcmds.c:2119
WARNING:  01000: snapshot 0x7fde12003038 still active
-- 
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] Documentation improvements for partitioning

[Amit Langote]

On 2017/02/10 19:19, Simon Riggs wrote:
> On 10 February 2017 at 08:18, Amit Langote
>  wrote:
> 
>> I agree that getting the proposed documentation changes committed would be
>> a step ahead.
> 
> Committed. I tested how it works and added documentation that matched
> my experiences, correcting what you'd said and adding more information
> for clarity as I went.

Thanks for making the improvements to and committing the patch!

> Few points from tests
> 
> * "ERROR:  cannot create index on partitioned table "measurement_year_month""
> is misleading because you can create indexes on partitions

Do you mean that this should not cause an error at all, but create the
specified index on partitions as part of running the command?  Should the
code to handle that be part of this release?

Or do you simply mean that we should rewrite the error message and/or add
a HINT asking to create the index on partitions instead?

> * You can create additional CHECK (column is NOT NULL) as a check
> constraint, even if you can't create a not null constraint

Sorry but I am not exactly sure which "cannot create a not null
constraint" you are referring to here.

There are no restrictions on *creating* constraints on partitions, but
there are on dropping.  Regular inheritance rules prevent dropping
inherited constraints (just the same for partitioned tables), of which
there are only named CHECK constraints at the moment.  A new rule
introduced for partitions prevents dropping a column's NOT NULL constraint
if it's been "inherited" (i.e., the same constraint is present in the
parent partitioned table), although it's not in the same sense as for
CHECK constraints, because NOT NULL constraints are not tracked with
pg_constraints.

> * The OID inheritance needs work - you shouldn't need to specify a
> partition needs OIDS if the parent has it already.

That sounds right.  It's better to keep the behavior same as for regular
inheritance.  Will post a patch to get rid of the unnecessary check.

FWIW, the check was added to prevent the command from succeeding in the
case where WITHOUT OIDS has been specified for a partition and the parent
partitioned table has the OID column.  Regular inheritance simply
*overrides* the WITHOUT OIDS specification, which might be seen as surprising.

> * There's no tab completion, which prevents people from testing this
> (maybe better now with some docs)

Will post a patch as well.

> * ERROR:  no partition of relation "measurement_year_month" found for row
> DETAIL:  Failing row contains (2016-12-02, 1, 1).
> should not return the whole row, just the partition keys

I think that makes sense.  Something along the lines of
BuildIndexValueDescription() for partition keys will be necessary.  Will
post a patch.

> * It's very weird you can't DROP a partitioned table. I think you need
> to add dependencies.

Do you mean it should be possible to DROP a partitioned table without
needing to specify CASCADE?  Currently, same thing happens for a
partitioned table as will for a inheritance parent.

> * ERROR:  TO must specify exactly one value per partitioning column
> should say something more like "you specified one column value,
> whereas the partitioning key requires two columns"

Should that be a DETAIL or HINT message?

> Good work so far, but there is still a very significant amount of work
> to do. And as this feature evolves it must now contain full
> documentation at every step, otherwise it won't be able to receive
> full and fair review. So please make sure each new patch contains docs
> changes for that patch.

Agreed that comprehensive documentation of any new feature is crucial both
during development and after the feature is released.

Thanks,
Amit




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

Re: [HACKERS] Potential data loss of 2PC files

[Michael Paquier]

On Tue, Jan 31, 2017 at 11:07 AM, Michael Paquier
 wrote:
> On Mon, Jan 30, 2017 at 10:52 PM, Heikki Linnakangas  wrote:
>> If that can happen, don't we have the same problem in many other places?
>> Like, all the SLRUs? They don't fsync the directory either.
>
> Right, pg_commit_ts and pg_clog enter in this category.

Implemented as attached.

>> Is unlink() guaranteed to be durable, without fsyncing the directory? If
>> not, then we need to fsync() the directory even if there are no files in it
>> at the moment, because some might've been removed earlier in the checkpoint
>> cycle.
>
> Hm... I am not an expert in file systems. At least on ext4 I can see
> that unlink() is atomic, but not durable. So if an unlink() is
> followed by a power failure, the previously unlinked file could be
> here if the parent directory is not fsync'd.

So I have been doing more work on this patch, with the following things done:
- Flush pg_clog, pg_commit_ts and pg_twophase at checkpoint phase to
ensure their durability.
- Create a durable_unlink() routine to give a way to perform a durable
file removal.
I am now counting 111 calls to unlink() in the backend code, and
looking at all of them most look fine with plain unlink() if they are
not made durable as they work on temporary files (see timeline.c for
example), with some exceptions:
- In pg_stop_backup, the old backup_label and tablespace_map removal
should be durable to avoid putting the system in a wrong state after
power loss. Other calls of unlink() are followed by durable_rename so
they are fine if let as such.
- Removal of old WAL segments should be durable as well. There is
already an effort to rename them durably in case of a segment
recycled. In case of a power loss, a file that should have been
removed could remain in pg_xlog.

Looking around, I have bumped as well on the following bug report for
SQlite which is in the same category of things:
http://sqlite.1065341.n5.nabble.com/Potential-bug-in-crash-recovery-code-unlink-and-friends-are-not-synchronous-td68885.html
Scary to see that in this case durability can be a problem at
transaction commit...
-- 
Michael


unlink-2pc-loss-v3.patch
Description: Binary data

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

[HACKERS] Keep ECPG comment for log_min_duration_statement

[Okano, Naoki]

Hi,

I'd like to make it easier to analyze slow queries in ECPG using 
log_min_duration_statement. Especially when DBA and C application developer 
is different, DBA can feedback slow embedded query to the developer without 
difficulty and mistakes.

When our customers log and look into slower queries in C programs with 
embedded SQL, they use log_min_duration_statement.Using this logging option, 
SQL statement slower than a threshold will be displayed, but comments won't. 
That's because the pre-compiler (ECPG) removes all the comments when the 
pre-compiler converts C with embedded SQL to normal C code.

Without comments, DBA has difficulty with identifying to which C code 
the slow query belongs. And the exact slow query issue cannot be reported 
to the developer.   
So, I'd like to modify ecpg command not to remove some specific comments.

[Use-cases]
Here is a scenario:
1) Writing comments to embedded C file
  a) Describe the detailed usage of SQL in each comment.
  
  b) Allocating id to each SQL.
 - application developer need to create corresponding table between id 
   and the detailed description of SQL
   
2) DBA takes advantage of comments especially when:
  a) Similar comments are displayed here and there. 
 In such a case, each comment plays a role as an identifier and makes it 
 easier for DBA to identify SQL he/she looking for.
  b) DBA and C application developer are different.
 DBA can tell an application developer which query is slow without mistakes.
  
[Interface]
add a new option "--enable-parse-comment" to ecpg command.
 
  ecpg --enable-parse-comment ,..., prog.pgc
 
This option enables ecpg command to pass on block comments (/* 〜 */) to 
converted C file.
The conditions to enable processing "block comments" as follows:
 - a block comment can be used with SELECT, INSERT, UPDATE, or DELETE
 - a block comment can be placed right after keywords: SELECT, INSERT, UPDATE, 
DELETE or With
 - other than those above error will occur
 - line comment(--) are ignored, which is same as log output when logging libpq 
application 
 
[Example]
1)[Correct comment position] this comment position is right after SELECT
 EXEC SQL SELECT /* qid=3, at line 30 in yourApp.ecpg */ * INTO :C1, :C2 FROM 
T1 WHERE C1=1;
 
2)[Incorrect comment position] this comment position is bad（error will occur)
 EXEC SQL /* qid=3, at line 30 in yourApp.ecpg */ SELECT * INTO :C1, :C2 FROM 
T1 WHERE C1=1;

As far as I searched, there seems no discussion on this topic.
Please let me know if exists.

Regards,
Okano Naoki
Fujitsu


-- 
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] Commit fest 2017-01 will begin soon!

[Jim Nasby]

On 2/1/17 10:39 PM, Michael Paquier wrote:

On Thu, Feb 2, 2017 at 1:23 PM, Amit Kapila  wrote:

Many thanks to you for running the show.  I think it might be okay if
one consolidated mail is sent for all the patches that are marked
"Returned with Feedback" or "Rejected" or "Moved to next CF".  OTOH,
there is some value in sending a separate mail for each patch so that
people can argue on the decision by CFM for one particular patch, but
not sure if it worth.


People tend to look at emails they are directly on in CC, that's why I
prefer sending individual emails.


BTW, the app also sends email notifications to anyone named on a patch 
(or at least I got notifications). Having a human spend time doing that 
by hand seems pretty silly.


Since the app also knows about the email threads, presumably it could 
send notifications to relevant threads as well.

--
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] Time to up bgwriter_lru_maxpages?

[Jim Nasby]

On 2/3/17 7:34 PM, Andres Freund wrote:

On 2017-02-03 19:26:55 -0600, Jim Nasby wrote:

On 2/3/17 6:20 PM, Andres Freund wrote:

- The ringbuffers in shared buffers can be problematic. One possible way of
solving that is to get rid of ringbuffers entirely and rely on different
initial values for usage_count instead, but that's not desirable if it just
means more clock sweep work for backends.

I'm not quite sure which ringbuffer you're referring to here? If to the
new one, why is it problematic?


No, I mean the non-default BufferAccessStrategy's.


That's not a ringbuffer that's a buffer ring ;)


In my defense, there's at least one mention of 'ring buffer' in 
storage/buffers. I thought there were more.


Yet more reason to just get rid of them ;P
--
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] 'text' instead of 'unknown' in Postgres 10

[Jim Nasby]

On 2/7/17 9:16 AM, Daniele Varrazzo wrote:

Thank you for the clarification: I will assume the behaviour cannot be
maintained on PG 10 and think whether the treatment of '{}' is too
magical and drop it instead.


BTW, I would hope that passing '{}' into a defined array field still 
works, since an empty array isn't treated the same as NULL, which means 
you need some way to create an empty array.

--
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] drop support for Python 2.3

[Jim Nasby]

On 2/7/17 10:49 PM, Tom Lane wrote:

Peter Eisentraut  writes:

I would like to propose that we drop support for Python 2.3.
...
We do have buildfarm coverage on prairiedog.  However, that runs a >10
year old operating system, so I think it is not representing real usage.

I have no particular objection to dropping 2.3 support, but should we
make some effort to fail gracefully (ie, with a relevant error message)
on older versions?  I would guess that the effect of your patch will be
to produce quite opaque failures.  We seem to be computing python_version
in configure, so it shouldn't be that hard to check.


Except AFAIK that won't protect the user if something on the OS changes 
and backends suddenly start loading a 2.3 library, though I guess pl/tcl 
suffers the same problem.


BTW, the easy way to check this (at least with cypthon) would be 
PY_VERSION_HEX >= 0x0204


(from cpython/include/patchlevel.h)
/* Version as a single 4-byte hex number, e.g. 0x010502B2 == 1.5.2b2.
   Use this for numeric comparisons, e.g. #if PY_VERSION_HEX >= ... */
#define PY_VERSION_HEX ((PY_MAJOR_VERSION << 24) | \
(PY_MINOR_VERSION << 16) | \
(PY_MICRO_VERSION <<  8) | \
(PY_RELEASE_LEVEL <<  4) | \
(PY_RELEASE_SERIAL << 0))
--
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] Removal of deprecated views pg_user, pg_group, pg_shadow

[Jim Nasby]

On 2/10/17 12:04 AM, Stephen Frost wrote:

They're misleading by having an arbitrary subset of the role attributes
and implying that the role relationships are simpler than they actually
are.  Frankly, they're also not being consistently maintained based on
any proper policy, which I find quite objectionable.


+1


Of course, we could fix these issues- we could add the grantor to the
pg_groups view, and perhaps even change it to be an acyclic directed
graph structure, and we could add the role attributes to pg_user and
pg_shadow which are missing, but at that point all we're really doing,
it seems to me, is providing synonyms for the existing canonical views,
and that hardly seems useful.


Well, there's always the issue of breaking peoples existing code, which 
will probably remain an issue until we become more "in your face" with 
users about stuff we're trying to deprecate.


My vote would be to either kill the views or explicitly deprecate them 
and move them to contrib.

--
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] Checksums by default?

[Tomas Vondra]

On 02/13/2017 02:29 AM, Jim Nasby wrote:

On 2/10/17 6:38 PM, Tomas Vondra wrote:

And no, backups may not be a suitable solution - the failure happens on
a standby, and the page (luckily) is not corrupted on the master. Which
means that perhaps the standby got corrupted by a WAL, which would
affect the backups too. I can't verify this, though, because the WAL got
removed from the archive, already. But it's a possibility.


Possibly related... I've got a customer that periodically has SR replias
stop in their tracks due to WAL checksum failure. I don't think there's
any hardware correlation (they've seen this on multiple machines).
Studying the code, it occurred to me that if there's any bugs in the
handling of individual WAL record sizes or pointers during SR then you
could get CRC failures. So far every one of these occurrences has been
repairable by replacing the broken WAL file on the replica. I've
requested that next time this happens they save the bad WAL.


I don't follow. You're talking about WAL checksums, this thread is about 
data checksums. I'm not seeing any WAL checksum failure, but when the 
standby attempts to apply the WAL (in particular a Btree/DELETE WAL 
record), it detects an incorrect data checksum in the underlying table.


So either there's a hardware issue, or the heap got corrupted by some 
preceding WAL. Or maybe one of the tiny gnomes in the CPU got tired and 
punched the bits wrong.


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

Re: [HACKERS] gitlab post-mortem: pg_basebackup waiting for checkpoint

[Jim Nasby]

On 2/11/17 4:36 AM, Michael Banck wrote:

I guess you're right, I've moved it further down. There is in fact a
message about the xlog location (unless you switch off wal entirely),
but having another one right before that mentioning the completed
checkpoint sounds ok to me.


1) I don't think this should be verbose output. Having a program sit 
there "doing nothing" for no apparent reason is just horrible UI design.


2) I think it'd be useful to have a way to get the status of a running 
checkpoint. The checkpointer already has that info, and I think it might 
even be in shared memory already. If there was a function that reported 
checkpoint status pg_basebackup could poll that to provide users with 
live status. That should be a separate patch though.

--
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] log_autovacuum_min_duration doesn't log VACUUMs

[Jim Nasby]

On 2/10/17 2:33 PM, Robert Haas wrote:

That having been said, I think it could certainly be useful to have
more control over what DDL gets logged in foreground processes.


FWIW, this is a significant problem outside of DDL. Once you're past 1-2 
levels of nesting SET client_min_messages = DEBUG becomes completely 
useless.


I think the ability to filter logging based on context would be very 
valuable. AFAIK you could actually do that for manual logging with 
existing plpgsql support, but obviously that won't help for anything else.

--
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] removing tsearch2

[Jim Nasby]

On 2/10/17 2:24 PM, Andrew Dunstan wrote:

There's a bunch of these things in /contrib which really ought to be
PGXN extensions (also CUBE, earthdistance, etc.).  However, one of the
steps in that would be getting the mainstream platforms to package them
so that users have a reasonable upgrade path, so I would not propose
doing it for 10.


Part of the reason for keeping a number of extensions is that it helps
test our extension infrastructure. Also they server as good pieces of
example code. So I don't want to get rid of them all, or even any of
them that have any degree of significant use. I think these days
tsearch2 is very largely redundant, so that means there's a good reason
not to keep it. But that's not true of cube, isn etc.


That's based on an assumption that PGXN shouldn't be treated as part of 
the community effort, which I think is a mistake. Having a robust, 
community run extension/package/module framework has proven to be 
extremely valuable for other programming environments, and IMHO we 
should be striving to improve in that area.

--
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] Should we cacheline align PGXACT?

[Jim Nasby]

On 8/22/16 12:24 AM, Andres Freund wrote:

Somewhat naïve question from someone with much less clue about low level
cache behaviour trying to follow along: given that we determine such
padding at compile time, how do we ensure that the cacheline size we're
targeting is right at runtime?

There's basically only very few common cacheline sizes. Pretty much only
64 byte and 128 bytes are common these days. By usually padding to the
larger of those two, we waste a bit of memory, but not actually cache
space on platforms with smaller lines, because the padding is never
accessed.


Though, with an N-way associative cache 2x more padding than necessary 
cuts the amount you can fit into the cache by half. That could be 
meaningful in some cases.

--
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] Checksums by default?

[Jim Nasby]

On 2/10/17 6:38 PM, Tomas Vondra wrote:

And no, backups may not be a suitable solution - the failure happens on
a standby, and the page (luckily) is not corrupted on the master. Which
means that perhaps the standby got corrupted by a WAL, which would
affect the backups too. I can't verify this, though, because the WAL got
removed from the archive, already. But it's a possibility.


Possibly related... I've got a customer that periodically has SR replias 
stop in their tracks due to WAL checksum failure. I don't think there's 
any hardware correlation (they've seen this on multiple machines). 
Studying the code, it occurred to me that if there's any bugs in the 
handling of individual WAL record sizes or pointers during SR then you 
could get CRC failures. So far every one of these occurrences has been 
repairable by replacing the broken WAL file on the replica. I've 
requested that next time this happens they save the bad WAL.

--
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] Adding the optional clause 'AS' in CREATE TRIGGER

[Jim Nasby]

On 11/14/16 8:51 PM, Tom Lane wrote:

1. The impression I have is that most people write trigger functions
so that they can be shared across multiple uses.  That'd be impossible
with anonymous trigger function blocks.  So you'd be trading off merging
two commands into one, versus having to write out the whole body of the
trigger multiple times, which wouldn't be much of a win.


I've fairly frequently needed one-off triggers, usually to do some kind 
of data validation that wasn't suitable in a CHECK constraint. Enough so 
that twice now[1] I've created generic trigger functions that allow you 
to specify an arbitrary expression to test against NEW and OLD.


1: Second, WIP example: 
https://github.com/decibel/tg_sanity/blob/master/sql/tg_sanity.sql

--
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] Improve OR conditions on joined columns (common star schema problem)

[Jim Nasby]

On 2/12/17 5:06 PM, David Rowley wrote:

Yet I've worked with OLTP applications
since 2005, and I struggle to recall any many:many joins at all.


Interesting... I've run across it numerous times. In any case, for OLTP 
there's other things you can do fairly easily.



Perhaps this optimisation is a candidate for only being applied when
some sort of planner_strength GUC (as mentioned in FOSDEM developer
meeting in 2016) reaches some threshold. There's certainly already
some planner smarts that can be skipped when such a GUC is set to a
lower level (e.g join removal). We could likely save many cycles if we
had the ability to re-plan queries where total_cost > X with more
smarts enabled.


Yeah, I strongly suspect some kind of "multi-stage" planner would be a 
big win.


As for the POC, that's the same kind of plan I'm seeing IRL: a nested 
loop gets used essentially to do the lookup of dimension text to 
dimension ID.


I'm wondering if there's any tricks that could be applied on the sort 
since it's dealing with CTIDs.


I do think it'd be even better if we had the ability to do that lookup 
as part of planning, so you could discover the exact stats for the 
relevant ID values, but that's even more involved.

--
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] Adding the optional clause 'AS' in CREATE TRIGGER

[Okano, Naoki]

On Thursday, February 9, 2017 4:41 AM Robert Haas wrote:
> You should add this to the next CommitFest so it doesn't get missed.
> 
> https://commitfest.postgresql.org/

Thank you for your kind advice!
I have added this patch to the CommitFest 2017-03. 

Regards,
Okano Naoki
Fujitsu

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

[HACKERS] Small improvement to parallel query docs

[David Rowley]

Tomas Vondra pointed out to me that there's an error in parallel.sgml
which confuses the inner and outer sides of the join.

I've attached a patch which fixes this, although I think I'm still
missing the point to text's explanation of why Merge Join is not
included due to it having to sort the inner side in each worker. Hash
join must build a hash table for each worker, so why is that OK by
sorting is not?

Anyway, I've attached a patch which fixes the outer/inner confusion
and cleans up a couple of other grammar mistakes and an omissions
regarding DISTINCT and ORDER BY not being supported in parallel
aggregate. I ended up rewriting a section too which was explaining
parallel aggregate, which I personally believe is a bit more clear to
read, but someone else may think otherwise.

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


parallel_doc_fixes.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] Improve OR conditions on joined columns (common star schema problem)

[David Rowley]

On 13 February 2017 at 06:32, Tom Lane  wrote:
> It's not so much poor choices as the cost of the optimization attempt ---
> if there's a K-relation OR clause, this will increase the cost of planning
> by a factor approaching K+1, whether or not you get a better plan out of
> it.  I ran the regression tests with some instrumentation and determined
> that this logic fires a dozen or two times, and fails to produce a plan
> that looks cheaper than the standard plan in any of those cases.  So if we
> go down this road, not only do we need a GUC but I suspect it had better
> default to off; only people using star schemas are really likely to get a
> win out of it.

I always try to shy away from assuming that the regression test suite
is a good reflection of a real world set of queries. It's full of
tests for edge cases that are rarely seen in reality. FWIW I did a
similar experiment with unique joins and was disappointed to see that
it didn't apply in more cases. Yet I've worked with OLTP applications
since 2005, and I struggle to recall any many:many joins at all.

Perhaps this optimisation is a candidate for only being applied when
some sort of planner_strength GUC (as mentioned in FOSDEM developer
meeting in 2016) reaches some threshold. There's certainly already
some planner smarts that can be skipped when such a GUC is set to a
lower level (e.g join removal). We could likely save many cycles if we
had the ability to re-plan queries where total_cost > X with more
smarts enabled.

-- 
 David Rowley   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] multivariate statistics (v19)

[David Fetter]

On Sun, Feb 12, 2017 at 10:35:04AM +, Dean Rasheed wrote:
> On 11 February 2017 at 01:17, Tomas Vondra  
> wrote:
> > Thanks for the feedback, I'll fix this. I've allowed myself to be a bit
> > sloppy because the number of attributes in the statistics is currently
> > limited to 8, so the overflows are currently not an issue. But it doesn't
> > hurt to make it future-proof, in case we change that mostly artificial limit
> > sometime in the future.
> >
> 
> Ah right, so it can't overflow at present, but it's neater to have an
> overflow-proof algorithm.
> 
> Thinking about the exactness of the division steps is quite
> interesting. Actually, the order of the multiplying factors doesn't
> matter as long as the divisors are in increasing order. So in both my
> proposal:
> 
> result = 1
> for (i = 1; i <= k; i++)
> result = (result * (n-k+i)) / i;
> 
> and David's proposal, which is equivalent but has the multiplying
> factors in the opposite order, equivalent to:
> 
> result = 1
> for (i = 1; i <= k; i++)
> result = (result * (n-i+1)) / i;
> 
> the divisions are exact at each step. The first time through the loop
> it divides by 1 which is trivially exact. The second time it divides
> by 2, having multiplied by 2 consecutive factors, one of which is
> therefore guaranteed to be divisible by 2. The third time it divides
> by 3, having multiplied by 3 consecutive factors, one of which is
> therefore guaranteed to be divisible by 3, and so on.

Right.  You know you can use integer division, which make sense as
permutations of discrete sets are always integers.

> My approach originally seemed more logical to me because of the way it
> derives from the recurrence relation binomial(n, k) = binomial(n-1,
> k-1) * n / k, but they both work fine as long as they have suitable
> overflow checks.

Right.  We could even cache those checks (sorry) based on data type
limits by architecture and OS if performance on those operations ever
matters that much.

> It's also interesting that descriptions of this algorithm tend to
> talk about setting k to min(k, n-k) at the start as an optimisation
> step, as I did in fact, whereas it's actually more than that -- it
> helps prevent unnecessary intermediate overflows when k > n/2. Of
> course, that's not a worry for the current use of this function, but
> it's good to have a robust algorithm.

Indeed. :)

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] WIP: About CMake v2

[Vladimir Rusinov]

On Fri, Feb 10, 2017 at 5:07 PM, Robert Haas  wrote:

> But also, I'm not really sure whether this conversion makes sense.  I
> mean, any build system is going to require some work, and accordingly
> our present build systems require some work.  cmake will require
> different work, but not necessarily less.  The current system has a
> long history; we pretty much know it works.  Switching will inevitably
> break some things.  Maybe we'll end up better off in the long term,
> but maybe we won't.  Things are pretty good now, so it seems like it
> would be easy for them to get worse rather than better.  If nothing
> else, everybody who has to learn the new system either to use it for
> development or because they are doing packaging will have to do some
> amount of extra work as a result of any switch.
>

For what it's worth (even well-maintained) cmake is a usability regression
from well-maintained autotools from syseng/packager perspective.

Two anecdotes:

- ./configure is much nicer from user perspective. Compare:
./configure --prefix=/bla --enable-foo --disable-bar --with-ssl=/opt/myssl
cmake -DCMAKE_INSTALL_PREFIX:PATH=/usr -DCMAKE_BLAH -DCMAKE_FOO
-DCMAKE_WHO_KNOWS_WHAT:PATH=/opt/myssl

- Things like set(_PYTHON3_VERSIONS 3.7 3.6 3.5 3.4 3.3 3.2 3.1 3.0)

Guess what, on older cmake versions this list did not include anything
older that 3.3, so it was failing with in-comprehensive generic "not found"
error even though 3.4 was installed.
With this "fix" somebody somewhere will be banging their head against the
wall again once 3.8 is out.
Overall, when things go wrong debugging cmake requires cmake knowledge,
while autotools mostly require shell knowledge which is much more common
(again, for sysadmins/packagers).

So while cmake is better for developers it is usually worse off for
packagers and advanced users. I'm not saying migration is not worth it, I'm
pointing out costs of such migration.

PS: I personally like Google's internal version of https://bazel.build/ a
lot. I've never used open-source version but I presume it's similar. While
it has many problems (Java, lack of popular IDE support, lack of popularity
and, again, Java) good parts are rules are both machine- and human-
readable and writable and generally easy to debug. I'm not bold enough to
propose PostgreSQL to use it, but I'd be happy to see ideas from it to be
used elsewhere.

-- 
Vladimir Rusinov
Storage SRE, Google Ireland

Google Ireland Ltd.,Gordon House, Barrow Street, Dublin 4, Ireland
Registered in Dublin, Ireland
Registration Number: 368047


smime.p7s
Description: S/MIME Cryptographic Signature

Re: [HACKERS] Improve OR conditions on joined columns (common star schema problem)

[Tom Lane]

David Rowley  writes:
> This is very interesting. Couldn't this be even more generic and
> instead of looking at just the jointree quals, also look at the join
> conditions too, as I think you can use this to also transforms queries
> such as:

The hard part of that is to remember exactly where you need to do surgery
on the querytree to replace the OR clause, keeping in mind that a tree
copy step is going to happen in between first searching for the OR and
doing the replacement.  I'm sure it's soluble but it would've involved
more complexity than I felt justified for a POC.

> I imagine you'd also want an enable_unionor GUC which can be used to
> disable this for when the planner makes a poor choice.

It's not so much poor choices as the cost of the optimization attempt ---
if there's a K-relation OR clause, this will increase the cost of planning
by a factor approaching K+1, whether or not you get a better plan out of
it.  I ran the regression tests with some instrumentation and determined
that this logic fires a dozen or two times, and fails to produce a plan
that looks cheaper than the standard plan in any of those cases.  So if we
go down this road, not only do we need a GUC but I suspect it had better
default to off; only people using star schemas are really likely to get a
win out of it.

Maybe we could improve that situation by applying some heuristics that
prevent the optimization from being attempted unless it's more likely to
produce a win.  But I'm not sure what that would look like.  We have to
make the decision pretty early and without a lot of information.  As an
example, Jim's case fails completely if we don't do the transformation
before reduce_outer_joins, because one of the things that *has* to happen
to get a desirable plan is to recognize that the extracted restriction
clause allows the left join to the dimension table to be simplified to an
inner join.

Having written the POC, I find myself not terribly satisfied with it.
It seems brute-force and not at all integrated with the rest of the
planner.  Still, asking for integration with the existing UNION planning
is probably silly, since that really needs to be thrown away and rewritten.
Maybe this is a reasonable way forward until we get a clearer view of
what that area needs to look like.

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] RADIUS fallback servers

[Magnus Hagander]

In a discussion at
https://www.postgresql.org/message-id/55d51b54.4050...@joh.to we talked
about adding RADIUS fallback servers. It never got to the point of it being
done.

PFA a patch that implements this.

It supports multiple RADIUS servers. For all other parameters (secret,
port, identifier) one can specify either the exact same number of entries,
in which case each server gets it's own, or exactly one entry in which case
that entry will apply to all servers. (Or zero entries for everything
except secret, which will make it the default).

Each server is tried in order. If it responds positive, auth is OK. If it
responds negative, auth is rejected. If it does not respond at all, we move
on to the next one.

I'm wondering if in doing this we should also make the RADIUS timeout a
configurable as HBA option, since it might become more important now?

-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/
diff --git a/doc/src/sgml/client-auth.sgml b/doc/src/sgml/client-auth.sgml
index 231fc40..f1ecf95 100644
--- a/doc/src/sgml/client-auth.sgml
+++ b/doc/src/sgml/client-auth.sgml
@@ -1589,23 +1589,35 @@ host ... ldap ldapurl="ldap://ldap.example.net/dc=example,dc=net?uid?sub";

 

+Multiple RADIUS servers can be specified, in which case they will
+be tried sequentially. If a negative response is received from
+a server, the authentication will fail. If no response is received,
+the next server in the list will be tried. To specify multiple
+servers, put the names within quotes and separate the server names
+with a comma. If multiple servers are specified, all other RADIUS
+options can also be given as a comma separate list, to apply
+individual values to each server. They can also be specified as
+a single value, in which case this value will apply to all servers.
+   
+
+   
 The following configuration options are supported for RADIUS:
  
   
-   radiusserver
+   radiusservers

 
- The name or IP address of the RADIUS server to connect to.
+ The name or IP addresses of the RADIUS servers to connect to.
  This parameter is required.
 

   
 
   
-   radiussecret
+   radiussecrets

 
- The shared secret used when talking securely to the RADIUS
+ The shared secrets used when talking securely to the RADIUS
  server. This must have exactly the same value on the PostgreSQL
  and RADIUS servers. It is recommended that this be a string of
  at least 16 characters. This parameter is required.
@@ -1623,17 +1635,17 @@ host ... ldap ldapurl="ldap://ldap.example.net/dc=example,dc=net?uid?sub";
   
 
   
-   radiusport
+   radiusports

 
- The port number on the RADIUS server to connect to. If no port
+ The port number on the RADIUS servers to connect to. If no port
  is specified, the default port 1812 will be used.
 

   
 
   
-   radiusidentifier
+   radiusidentifiers

 
  The string used as NAS Identifier in the RADIUS
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index 824e408..e033b24 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -195,6 +195,7 @@ static int pg_SSPI_make_upn(char *accountname,
  *
  */
 static int	CheckRADIUSAuth(Port *port);
+static int	PerformRadiusTransaction(char *server, char *secret, char *portstr, char *identifier, char *user_name, char *passwd);
 
 
 /*
@@ -2455,7 +2456,97 @@ static int
 CheckRADIUSAuth(Port *port)
 {
 	char	   *passwd;
-	char	   *identifier = "postgresql";
+	ListCell   *server,
+			   *secrets,
+			   *radiusports,
+			   *identifiers;
+
+	/* Make sure struct alignment is correct */
+	Assert(offsetof(radius_packet, vector) == 4);
+
+	/* Verify parameters */
+	if (list_length(port->hba->radiusservers) < 1)
+	{
+		ereport(LOG,
+(errmsg("RADIUS server not specified")));
+		return STATUS_ERROR;
+	}
+
+	if (list_length(port->hba->radiussecrets) < 1)
+	{
+		ereport(LOG,
+(errmsg("RADIUS secret not specified")));
+		return STATUS_ERROR;
+	}
+
+	/* Send regular password request to client, and get the response */
+	sendAuthRequest(port, AUTH_REQ_PASSWORD, NULL, 0);
+
+	passwd = recv_password_packet(port);
+	if (passwd == NULL)
+		return STATUS_EOF;		/* client wouldn't send password */
+
+	if (strlen(passwd) == 0)
+	{
+		ereport(LOG,
+(errmsg("empty password returned by client")));
+		return STATUS_ERROR;
+	}
+
+	if (strlen(passwd) > RADIUS_MAX_PASSWORD_LENGTH)
+	{
+		ereport(LOG,
+(errmsg("RADIUS authentication does not support passwords longer than %d characters", RADIUS_MAX_PASSWORD_LENGTH)));
+		return STATUS_ERROR;
+	}
+
+	/*
+	 * Loop over and try each server in order.
+	 */
+	secrets = list_head(port->hba->radiussecrets

Re: [HACKERS] Improve OR conditions on joined columns (common star schema problem)

[David Rowley]

On 12 February 2017 at 13:30, Tom Lane  wrote:
> I wrote a POC patch for this on a long airplane ride.  It's not complete,
> and I'm sure there are bugs as well, but it makes your example case
> better.  What I did about the de-duplication issue is to de-dup using
> the CTIDs of all baserels in the query.  This means the optimization
> is only applicable to cases where all the rels have CTIDs ... but other
> methods such as inspecting unique keys ain't gonna work for non-table
> rels either, so I think this is about the best we can hope for.
> However, I did not understand your point about:

This is very interesting. Couldn't this be even more generic and
instead of looking at just the jointree quals, also look at the join
conditions too, as I think you can use this to also transforms queries
such as:

select * from t1 inner join t2 on t1.a = t2.a or t1.b = t2.b;

I imagine you'd also want an enable_unionor GUC which can be used to
disable this for when the planner makes a poor choice.

-- 
 David Rowley   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] multivariate statistics (v19)

[Dean Rasheed]

On 11 February 2017 at 01:17, Tomas Vondra  wrote:
> Thanks for the feedback, I'll fix this. I've allowed myself to be a bit
> sloppy because the number of attributes in the statistics is currently
> limited to 8, so the overflows are currently not an issue. But it doesn't
> hurt to make it future-proof, in case we change that mostly artificial limit
> sometime in the future.
>

Ah right, so it can't overflow at present, but it's neater to have an
overflow-proof algorithm.

Thinking about the exactness of the division steps is quite
interesting. Actually, the order of the multiplying factors doesn't
matter as long as the divisors are in increasing order. So in both my
proposal:

result = 1
for (i = 1; i <= k; i++)
result = (result * (n-k+i)) / i;

and David's proposal, which is equivalent but has the multiplying
factors in the opposite order, equivalent to:

result = 1
for (i = 1; i <= k; i++)
result = (result * (n-i+1)) / i;

the divisions are exact at each step. The first time through the loop
it divides by 1 which is trivially exact. The second time it divides
by 2, having multiplied by 2 consecutive factors, one of which is
therefore guaranteed to be divisible by 2. The third time it divides
by 3, having multiplied by 3 consecutive factors, one of which is
therefore guaranteed to be divisible by 3, and so on.

My approach originally seemed more logical to me because of the way it
derives from the recurrence relation binomial(n, k) = binomial(n-1,
k-1) * n / k, but they both work fine as long as they have suitable
overflow checks.

It's also interesting that descriptions of this algorithm tend to talk
about setting k to min(k, n-k) at the start as an optimisation step,
as I did in fact, whereas it's actually more than that -- it helps
prevent unnecessary intermediate overflows when k > n/2. Of course,
that's not a worry for the current use of this function, but it's good
to have a robust algorithm.

Regards,
Dean


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

[Fabien COELHO]

Hello Greg,


So you'd just want to know nesting depth, with no indicator of true/false?


Even that's more than bash does, for example: [...]


Indeed, there is nothing in "bash" prompt about nesting control 
structures. However other shells have such indications: "zsh" has "%_", 
"tcsh" has "%R". In tcsh for example, there is mention of the structure 
type but none of nesting depth nor truth:


  >if ( 0 ) then
  if?...


I'm a bit confused how the true/false is actually valuable.


The point is just to tell the user that the next command (1) is under an 
if control structure and (2) whether it is going to be executed or 
ignored. That is not too bad in 2 characters.



It doesn't tell you how the expression actually evaluated,


I do not get your point... t tells that it was true, f that it was false?

just where you are in the code you're typing in which you can tell 
equally well by looking at what code you're typing in.


  SELECT ... AS condition \gset
  \if :condition ...

The value of the condition is not obvious from the code, it depends on the 
database state.


The reason nesting level is handy is just to remind you in case you 
forget.


Sure, that can be useful too.


For debugging scripts it would be handy to have some way to tell
whether the \if expression actually evaluated to true or false but
that wouldn't be in the prompt I don't think.


Are you suggest to add another command to display the current stack state, 
eg "\ifstate" or whatever?


"\if" is really about scripting, so the idea was to have something quite 
light for interactive debugging, especially to help the user not to be 
stuck into a false branch, hence the prompt information with t/f/z.


What should be in the prompt is indeed debatable: existence, nesting 
depth, current truth value, part of the stack... I think that something, 
whatever it is, is necessary.


Maybe this can be a discussed in a follow-up patch and Corey should 
proceed to finalize the if patch?


--
Fabien


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