pg_dump outputs invalid syntax for partitions with bool partkeys in pg10

Started by David Rowleyalmost 8 years ago2 messages
#1David Rowley
david.rowley@2ndquadrant.com
1 attachment(s)

Quite simply:

d1=# create table bp (b bool) partition by list(b);
CREATE TABLE
d1=# create table bp_f partition of bp for values in('f');
CREATE TABLE
d1=# \q
$ createdb d2
$ pg_dump d1 | psql d2

...

ERROR: syntax error at or near "false"
LINE 2: FOR VALUES IN (false);
^
ERROR: relation "public.bp_f" does not exist
ERROR: relation "public.bp_f" does not exist
invalid command \.

I understand there's a thread [1]/messages/by-id/e05c5162-1103-7e37-d1ab-6de3e0afaf70@lab.ntt.co.jp which is discussing making this
valid syntax, but on skimming it, there's no mention of any bugs.

I'd thought about something like the attached patch (against master)
to fix, but that leaves pg_dumps that have already been taken a bit
out in the cold, so perhaps we need to think harder about allowing
this syntax.

Thoughts?

This was noticed by Jesper Pedersen in [2]/messages/by-id/c3eb3284-24c8-eff0-f930-a33984d1168a@redhat.com, but I've diagnosed this as
an existing bug rather than a bug in the patch that thread relates to.

[1]: /messages/by-id/e05c5162-1103-7e37-d1ab-6de3e0afaf70@lab.ntt.co.jp

[2]: /messages/by-id/c3eb3284-24c8-eff0-f930-a33984d1168a@redhat.com

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

Attachments:

very_minimal_bool_partition_syntax_fix.patchapplication/octet-stream; name=very_minimal_bool_partition_syntax_fix.patchDownload
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index 3697466..f85bbc1 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -433,7 +433,7 @@ static void get_coercion_expr(Node *arg, deparse_context *context,
 				  Oid resulttype, int32 resulttypmod,
 				  Node *parentNode);
 static void get_const_expr(Const *constval, deparse_context *context,
-			   int showtype);
+			   int showtype, bool partition);
 static void get_const_collation(Const *constval, deparse_context *context);
 static void simple_quote_literal(StringInfo buf, const char *val);
 static void get_sublink_expr(SubLink *sublink, deparse_context *context);
@@ -5672,7 +5672,7 @@ get_rule_sortgroupclause(Index ref, List *tlist, bool force_colno,
 		appendStringInfo(buf, "%d", tle->resno);
 	}
 	else if (expr && IsA(expr, Const))
-		get_const_expr((Const *) expr, context, 1);
+		get_const_expr((Const *) expr, context, 1, false);
 	else if (!expr || IsA(expr, Var))
 		get_rule_expr(expr, context, true);
 	else
@@ -7675,7 +7675,7 @@ get_rule_expr(Node *node, deparse_context *context,
 			break;
 
 		case T_Const:
-			get_const_expr((Const *) node, context, 0);
+			get_const_expr((Const *) node, context, 0, false);
 			break;
 
 		case T_Param:
@@ -8779,7 +8779,7 @@ get_rule_expr(Node *node, deparse_context *context,
 							Const	   *val = castNode(Const, lfirst(cell));
 
 							appendStringInfoString(buf, sep);
-							get_const_expr(val, context, -1);
+							get_const_expr(val, context, -1, true);
 							sep = ", ";
 						}
 
@@ -9285,7 +9285,7 @@ get_coercion_expr(Node *arg, deparse_context *context,
 		((Const *) arg)->consttypmod == -1)
 	{
 		/* Show the constant without normal ::typename decoration */
-		get_const_expr((Const *) arg, context, -1);
+		get_const_expr((Const *) arg, context, -1, false);
 	}
 	else
 	{
@@ -9312,10 +9312,15 @@ get_coercion_expr(Node *arg, deparse_context *context,
  * We mustn't do this when showtype is -1 (since that means the caller will
  * print "::typename", and we can't put a COLLATE clause in between).  It's
  * caller's responsibility that collation isn't missed in such cases.
+ *
+ * 'partition' can be set to true to have bool values returned as 't' or 'f'
+ * instead of true or false.  This is required due to an omission in the
+ * partitioning syntax which does not correctly handle bool literals.
  * ----------
  */
 static void
-get_const_expr(Const *constval, deparse_context *context, int showtype)
+get_const_expr(Const *constval, deparse_context *context, int showtype,
+			   bool partition)
 {
 	StringInfo	buf = context->buf;
 	Oid			typoutput;
@@ -9393,9 +9398,9 @@ get_const_expr(Const *constval, deparse_context *context, int showtype)
 
 		case BOOLOID:
 			if (strcmp(extval, "t") == 0)
-				appendStringInfoString(buf, "true");
+				appendStringInfoString(buf, partition ? "'t'" : "true");
 			else
-				appendStringInfoString(buf, "false");
+				appendStringInfoString(buf, partition ? "'f'" : "false");
 			break;
 
 		default:
@@ -11043,7 +11048,7 @@ get_range_partbound_string(List *bound_datums)
 		{
 			Const	   *val = castNode(Const, datum->value);
 
-			get_const_expr(val, &context, -1);
+			get_const_expr(val, &context, -1, true);
 		}
 		sep = ", ";
 	}
#2Amit Langote
Langote_Amit_f8@lab.ntt.co.jp
In reply to: David Rowley (#1)
Re: pg_dump outputs invalid syntax for partitions with bool partkeys in pg10

Hi David.

On 2018/03/02 12:41, David Rowley wrote:

Quite simply:

d1=# create table bp (b bool) partition by list(b);
CREATE TABLE
d1=# create table bp_f partition of bp for values in('f');
CREATE TABLE
d1=# \q
$ createdb d2
$ pg_dump d1 | psql d2

...

ERROR: syntax error at or near "false"
LINE 2: FOR VALUES IN (false);
^
ERROR: relation "public.bp_f" does not exist
ERROR: relation "public.bp_f" does not exist
invalid command \.

I understand there's a thread [1] which is discussing making this
valid syntax, but on skimming it, there's no mention of any bugs.

Thanks for the report.

I guess we don't have a Boolean partitioned table in tests covered by
pg_dump or I'd have heard about this syntax glitch before Horiguchi-san
mentioned it a few months back [1]/messages/by-id/20171128.203915.26713586.horiguchi.kyotaro@lab.ntt.co.jp.

I'd thought about something like the attached patch (against master)
to fix, but that leaves pg_dumps that have already been taken a bit
out in the cold, so perhaps we need to think harder about allowing
this syntax.

I agree about going ahead with adding the syntax support, but there are
some arguments against the design of the proposed patches or rather about
the original design of partition bound parsing itself.

This was noticed by Jesper Pedersen in [2], but I've diagnosed this as
an existing bug rather than a bug in the patch that thread relates to.

Certainly.

Thanks,
Amit

[1]: /messages/by-id/20171128.203915.26713586.horiguchi.kyotaro@lab.ntt.co.jp
/messages/by-id/20171128.203915.26713586.horiguchi.kyotaro@lab.ntt.co.jp