Re: pl/pgsql feature request: shorthand for argument and local variable references

[Pavel Stehule]

st 17. 3. 2021 v 23:32 odesílatel Michael Paquier 
napsal:

> On Wed, Mar 17, 2021 at 05:04:48PM +0100, Pavel Stehule wrote:
> > This tree has a different direction than is usual, and then replacing the
> > root node is not simple.
>
> Yeah, it is not like we should redesign this whole part just for the
> feature discussed here, and that may impact performance as the current
> list handling is cheap now.
>
> > Second solution (significantly more simple) is an additional pointer in
> > ns_item structure. In almost all cases this pointer will not be used.
> > Because ns_item uses a flexible array, then union cannot be used. I
> > implemented this in a patch marked as "alias-implementation".
> >
> > What do you think about it?
>
> I am not sure that it is necessary nor good to replace entirely the
> root label.  So associating a new field directly into it sounds like a
> promising approach?
>

Here is rebased patch

I am continuing to  talk with Hannu about syntax. I think so using the
compile option is a good direction, but maybe renaming from "routine_label"
to "topscope_label" (proposed by Hannu) or maybe "outerscope_label" can be
a good idea.

Regards

Pavel


--
> Michael
>
diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml
index 52f60c827c..d375284a2c 100644
--- a/doc/src/sgml/plpgsql.sgml
+++ b/doc/src/sgml/plpgsql.sgml
@@ -292,7 +292,9 @@ $$ LANGUAGE plpgsql;
   special variables such as FOUND (see
   ).  The outer block is
   labeled with the function's name, meaning that parameters and special
-  variables can be qualified with the function's name.
+  variables can be qualified with the function's name. The name of this outer
+  block can be changed by inserting special command 
+  #routine_label new_name at the start of the function.
  
 
 
@@ -435,6 +437,31 @@ $$ LANGUAGE plpgsql;
  
 
 
+
+ The function's argument can be qualified with the function name:
+
+CREATE FUNCTION sales_tax(subtotal real) RETURNS real AS $$
+BEGIN
+RETURN sales_tax.subtotal * 0.06;
+END;
+$$ LANGUAGE plpgsql;
+
+
+
+
+ Sometimes the function name is too long and it can be more practical to use
+ some shorter label. The top namespace label can be changed (along with
+ the functions' arguments) using the option routine_label:
+
+CREATE FUNCTION sales_tax(subtotal real) RETURNS real AS $$
+#routine_label s
+BEGIN
+RETURN s.subtotal * 0.06;
+END;
+$$ LANGUAGE plpgsql;
+
+
+
  
   Some more examples:
 
diff --git a/src/pl/plpgsql/src/pl_comp.c b/src/pl/plpgsql/src/pl_comp.c
index ce8d97447d..d32e050c32 100644
--- a/src/pl/plpgsql/src/pl_comp.c
+++ b/src/pl/plpgsql/src/pl_comp.c
@@ -378,6 +378,10 @@ do_compile(FunctionCallInfo fcinfo,
 	 */
 	plpgsql_ns_init();
 	plpgsql_ns_push(NameStr(procStruct->proname), PLPGSQL_LABEL_BLOCK);
+
+	/* save top ns for possibility to alter top label */
+	function->root_ns = plpgsql_ns_top();
+
 	plpgsql_DumpExecTree = false;
 	plpgsql_start_datums();
 
diff --git a/src/pl/plpgsql/src/pl_funcs.c b/src/pl/plpgsql/src/pl_funcs.c
index 919b968826..7132da35d1 100644
--- a/src/pl/plpgsql/src/pl_funcs.c
+++ b/src/pl/plpgsql/src/pl_funcs.c
@@ -101,6 +101,7 @@ plpgsql_ns_additem(PLpgSQL_nsitem_type itemtype, int itemno, const char *name)
 	nse->itemtype = itemtype;
 	nse->itemno = itemno;
 	nse->prev = ns_top;
+	nse->alias = NULL;
 	strcpy(nse->name, name);
 	ns_top = nse;
 }
@@ -141,7 +142,7 @@ plpgsql_ns_lookup(PLpgSQL_nsitem *ns_cur, bool localmode,
 			 nsitem->itemtype != PLPGSQL_NSTYPE_LABEL;
 			 nsitem = nsitem->prev)
 		{
-			if (strcmp(nsitem->name, name1) == 0)
+			if (strcmp(nsitem->alias ? nsitem->alias : nsitem->name, name1) == 0)
 			{
 if (name2 == NULL ||
 	nsitem->itemtype != PLPGSQL_NSTYPE_VAR)
@@ -155,13 +156,13 @@ plpgsql_ns_lookup(PLpgSQL_nsitem *ns_cur, bool localmode,
 
 		/* Check this level for qualified match to variable name */
 		if (name2 != NULL &&
-			strcmp(nsitem->name, name1) == 0)
+			strcmp(nsitem->alias ? nsitem->alias : nsitem->name, name1) == 0)
 		{
 			for (nsitem = ns_cur;
  nsitem->itemtype != PLPGSQL_NSTYPE_LABEL;
  nsitem = nsitem->prev)
 			{
-if (strcmp(nsitem->name, name2) == 0)
+if (strcmp(nsitem->alias ? nsitem->alias : nsitem->name, name2) == 0)
 {
 	if (name3 == NULL ||
 		nsitem->itemtype != PLPGSQL_NSTYPE_VAR)
@@ -197,7 +198,7 @@ plpgsql_ns_lookup_label(PLpgSQL_nsitem *ns_cur, const char *name)
 	while (ns_cur != NULL)
 	{
 		if (ns_cur->itemtype == PLPGSQL_NSTYPE_LABEL &&
-			strcmp(ns_cur->name, name) == 0)
+			strcmp(ns_cur->alias ? ns_cur->alias : ns_cur->name, name) == 0)
 			return ns_cur;
 		ns_cur = ns_cur->prev;
 	}
diff --git a/src/pl/plpgsql/src/pl_gram.y b/src/pl/plpgsql/src/pl_gram.y
index 34e0520719..f3536d75ae 100644
--- a/src/pl/plpgsql/src/pl_gram.y
+++ b/src/pl/plpgsql/src/pl_gram.y
@@ -333,6 +333,7 @@ static	void			check_raise_parameters(PLpgSQL_stmt_raise *stmt);
 %token 	K_RETURNED_SQLS

RE: Logical Replication vs. 2PC

[tanghy.f...@fujitsu.com]

On Sunday, March 21, 2021 4:40 PM Amit Kapila  wrote:

>I have enhanced the patch for 2PC implementation on the
>subscriber-side as per the solution discussed here [1].

FYI.
I did the confirmation for the solution of unique GID problem raised at [1].
This problem in V61-patches at [2] is fixed in the latest V66-patches at [3].

B.T.W. NG log at V61-patches is attached, please take it as your reference.
   Test step is just the same as Amit said at [1].

[1] - 
https://www.postgresql.org/message-id/CAA4eK1+opiV4aFTmWWUF9h_32=hfpow9vzashart0ua5obr...@mail.gmail.com
[2] - 
https://www.postgresql.org/message-id/CAHut%2BPv3X7YH_nDEjH1ZJf5U6M6DHHtEjevu7PY5Dv5071jQ4A%40mail.gmail.com
[3] - 
https://www.postgresql.org/message-id/CAA4eK1JPEoYAkggmLqbdD%2BcF%3DkWNpLkZb_wJ8eqj0QD2AjBTBA%40mail.gmail.com

Regards,
Tang



subscriber.log
Description: subscriber.log

Re: [HACKERS] logical decoding of two-phase transactions

[Peter Smith]

On Tue, Mar 23, 2021 at 9:01 PM Peter Smith  wrote:
>
> On Tue, Mar 23, 2021 at 10:44 AM Peter Smith  wrote:
> >
> > On Mon, Mar 22, 2021 at 11:51 PM Amit Kapila  
> > wrote:
> > >
> > > I have incorporated all your changes and additionally made few more
> > > changes (a) got rid of LogicalRepBeginPrepareData and instead used
> > > LogicalRepPreparedTxnData, (b) made a number of changes in comments
> > > and docs, (c) ran pgindent, (d) modified tests to use standard
> > > wait_for_catch function and removed few tests to reduce the time and
> > > to keep regression tests reliable.
> >
> > I checked all v65* / v66* differences and found only two trivial comment 
> > typos.
> >
> > PSA patches to fix those.
> >
>
> Hi Amit.
>
> PSA a patch to allow the ALTER SUBSCRIPTION ... REFRESH PUBLICATION to
> work when two-phase tristate is PENDING.
>
> This is necessary for the pg_dump/pg_restore scenario, or for any
> other use-case where the subscription might
> start off having no tables.
>
> Please apply this on top of your v66-0001 (after applying the other
> Feedback patches I posted earlier today).
>

PSA a small addition to the 66-0003 "Fix to allow REFRESH PUBLICATION"
patch posted yesterday.

This just updates the worker.c comment.

--
Kind Regards,
Peter Smith.
Fujitsu Australia.


v66-0004-Updated-worker.c-comment.patch
Description: Binary data

Re: pl/pgsql feature request: shorthand for argument and local variable references

[Erik Rijkers]

> On 2021.03.24. 08:09 Pavel Stehule  wrote:
> [...]
> [plpgsql-routine-label-20210324.patch]


Hi,

In that sgml

"the functions' arguments"

should be

"the function's arguments"


Erik

Re: [HACKERS] Custom compression methods

[Jaime Casanova]

On Fri, Mar 19, 2021 at 2:44 PM Robert Haas  wrote:
>
> I committed the core patch (0003) with a bit more editing.  Let's see
> what the buildfarm thinks.
>

I think this is bbe0a81db69bd10bd166907c3701492a29aca294, right?
This introduced a new assert failure, steps to reproduce:

"""
create table t1 (col1 text, col2 text);
create unique index on t1 ((col1 || col2));
insert into t1 values((select array_agg(md5(g::text))::text from
generate_series(1, 256) g), version());
"""

Attached is a backtrace from current HEAD

-- 
Jaime Casanova
Director de Servicios Profesionales
SYSTEMGUARDS - Consultores de PostgreSQL
#0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
set = {__val = {4194304, 140726735034080, 2, 6, 6515006, 
94155769016304, 4611686018427388799, 140459852548774, 0, 281470681751456, 0, 0, 
0, 0, 0, 0}}
pid = 
tid = 
ret = 
#1  0x7fbf5b657535 in __GI_abort () at abort.c:79
save_stage = 1
act = {__sigaction_handler = {sa_handler = 0x0, sa_sigaction = 0x0}, 
sa_mask = {__val = {0, 0, 0, 0, 0, 140459850305525, 2, 3630241276386869248, 
7003431909949847861, 94155769016304, 
  7003772758621645568, 0, 1523958137291918592, 140726735034320, 0, 
140726735035184}}, sa_flags = 1495953392, sa_restorer = 0x0}
sigs = {__val = {32, 0 }}
#2  0x55a25992a983 in ExceptionalCondition (conditionName=0x55a2599a9248 
"CompressionMethodIsValid(cmethod)", errorType=0x55a2599a91d2 
"FailedAssertion", fileName=0x55a2599a91c0 "toast_internals.c", 
lineNumber=56) at assert.c:69
No locals.
#3  0x55a2592c0709 in toast_compress_datum (value=94155801611904, cmethod=0 
'\000') at toast_internals.c:56
tmp = 0x0
valsize = 1528257472
cmid = TOAST_INVALID_COMPRESSION_ID
__func__ = "toast_compress_datum"
#4  0x55a2592b9532 in index_form_tuple (tupleDescriptor=0x7fbf524881a8, 
values=0x7ffd7f0d5e10, isnull=0x7ffd7f0d5df0) at indextuple.c:106
cvalue = 140726735035664
att = 0x7fbf524881c0
tp = 0x25b19e830 
tuple = 0x55a25b1a4608
size = 94155801511432
data_size = 0
hoff = 0
i = 0
infomask = 0
hasnull = false
tupmask = 0
numberOfAttributes = 1
untoasted_values = {94155801611904, 94155771761321, 140726735035376, 
94155801506904, 94155779258656, 140726735035488, 94155779258656, 
140726735035488, 0, 140459848948121, 140726735035488, 
  94155779258656, 140726735035520, 0, 140726735035520, 94155769016304, 
94155801611904, 8589934592, 94155801512576, 94155801512544, 94155801511432, 
94155801506904, 94155771775058, 94155801352832, 0, 
  94155779258656, 6426549472, 94155771784441, 94155801487408, 0, 0, 0}
untoasted_free = {false, false, false, false, false, false, false, 
false, false, 4, false, false, false, false, false, false, 8, 69, 26, 91, 162, 
85, false, false, 192, 91, 23, 91, 162, 85, false, 
  false}
__func__ = "index_form_tuple"
#5  0x55a259355563 in btinsert (rel=0x7fbf5248b3f0, values=0x7ffd7f0d5e10, 
isnull=0x7ffd7f0d5df0, ht_ctid=0x55a25b19e860, heapRel=0x7fbf524840a8, 
checkUnique=UNIQUE_CHECK_YES, indexUnchanged=false, 
indexInfo=0x55a25b17d968) at nbtree.c:196
result = false
itup = 0x55a2593eb96d 
#6  0x55a259341253 in index_insert (indexRelation=0x7fbf5248b3f0, 
values=0x7ffd7f0d5e10, isnull=0x7ffd7f0d5df0, heap_t_ctid=0x55a25b19e860, 
heapRelation=0x7fbf524840a8, checkUnique=UNIQUE_CHECK_YES, 
indexUnchanged=false, indexInfo=0x55a25b17d968) at indexam.c:193
__func__ = "index_insert"
#7  0x55a259551d6b in ExecInsertIndexTuples (resultRelInfo=0x55a25b17d0a8, 
slot=0x55a25b19e830, estate=0x55a25b175ce0, update=false, noDupErr=false, 
specConflict=0x0, arbiterIndexes=0x0)
at execIndexing.c:416
applyNoDupErr = false
checkUnique = UNIQUE_CHECK_YES
indexRelation = 0x7fbf5248b3f0
indexInfo = 0x55a25b17d968
indexUnchanged = false
satisfiesConstraint = false
tupleid = 0x55a25b19e860
result = 0x0
i = 0
numIndices = 1
relationDescs = 0x55a25b17d900
heapRelation = 0x7fbf524840a8
indexInfoArray = 0x55a25b17d920
econtext = 0x55a25b1a2f88
values = {94155801611904, 94155801320384, 23002747632, 94155801505976, 
94155801505952, 94155801320384, 140726735036016, 94155776090255, 
94155800974064, 94155801505976, 140726735036048, 94155801320384, 
  140726735036048, 94155769089800, 5801161712, 94155801505976, 
140726735036192, 94155769471556, 0, 0, 94155801603232, 140459695947944, 
140726735036224, 6764221304040416, 1, 23351616, 140459705036672, 
  856226916400, 94155801505976, 2325076142599, 549755813894, 0}
isnull = {false, true, false, false, false, false, false, false, 184, 
48, 26, 91, 162, 85, false, false, 64, 94, 13, 127, 253, 127, false, false, 
135, 177, 149, 89, 162, 85, fals

Re: Failed assertion on standby while shutdown

[Fujii Masao]

On 2021/03/24 14:02, Maxim Orlov wrote:

Thank you for reply! As far as I understand, this is really the case. I've test 
your patch a bit.


Thanks for testing the patch!


This annoying failed assertion is gone now.


Good news!


I think I should test more and report later about results.

Should we put this patch to CF?


Yes. Since this is a bug, we can review and commit the patch
without waiting for next CF. But I agree that it's better to
add the patch to next CF so that we don't forget the patch.

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

Re: Nicer error when connecting to standby with hot_standby=off

[Alvaro Herrera]

On 2021-Mar-24, Fujii Masao wrote:

> On 2021/03/24 5:59, Tom Lane wrote:
> > Alvaro Herrera  writes:
> > > FATAL:  the database system is starting up
> > > DETAIL:  WAL is being applied to recover from a system crash.
> > > or
> > > DETAIL:  The system is applying WAL to recover from a system crash.
> > > or
> > > DETAIL:  The startup process is applying WAL to recover from a system 
> > > crash.
> > 
> > I don't think the postmaster has enough context to know if that's
> > actually true.  It just launches the startup process and waits for
> > results.  If somebody saw this during a normal (non-crash) startup,
> > they'd be justifiably alarmed.
> 
> Yes, so logging "the database system is starting up" seems enough to me.

No objection.

-- 
Álvaro Herrera   Valdivia, Chile
"Cuando no hay humildad las personas se degradan" (A. Christie)

Re: Feature improvement: can we add queryId for pg_catalog.pg_stat_activity view?

[Alvaro Herrera]

On 2021-Mar-22, Bruce Momjian wrote:

> diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
> index e259531f60..9550de0798 100644
> --- a/src/include/catalog/pg_proc.dat
> +++ b/src/include/catalog/pg_proc.dat
> @@ -5249,9 +5249,9 @@
>proname => 'pg_stat_get_activity', prorows => '100', proisstrict => 'f',
>proretset => 't', provolatile => 's', proparallel => 'r',
>prorettype => 'record', proargtypes => 'int4',
> -  proallargtypes => 
> '{int4,oid,int4,oid,text,text,text,text,text,timestamptz,timestamptz,timestamptz,timestamptz,inet,text,int4,xid,xid,text,bool,text,text,int4,text,numeric,text,bool,text,bool,int4}',
> -  proargmodes => 
> '{i,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o}',
> -  proargnames => 
> '{pid,datid,pid,usesysid,application_name,state,query,wait_event_type,wait_event,xact_start,query_start,backend_start,state_change,client_addr,client_hostname,client_port,backend_xid,backend_xmin,backend_type,ssl,sslversion,sslcipher,sslbits,ssl_client_dn,ssl_client_serial,ssl_issuer_dn,gss_auth,gss_princ,gss_enc,leader_pid}',
> +  proallargtypes => 
> '{int4,oid,int4,oid,text,text,text,text,text,timestamptz,timestamptz,timestamptz,timestamptz,inet,text,int4,xid,xid,text,bool,text,text,int4,text,numeric,text,bool,text,bool,int4,int8}',
> +  proargmodes => 
> '{i,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o}',
> +  proargnames => 
> '{pid,datid,pid,usesysid,application_name,state,query,wait_event_type,wait_event,xact_start,query_start,backend_start,state_change,client_addr,client_hostname,client_port,backend_xid,backend_xmin,backend_type,ssl,sslversion,sslcipher,sslbits,ssl_client_dn,ssl_client_serial,ssl_issuer_dn,gss_auth,gss_princ,gss_enc,leader_pid,queryid}',

BTW why do you put the queryid column at the end of the column list
here?  It seems awkward.  Can we put it perhaps between state and query?


> -const char *clean_querytext(const char *query, int *location, int *len);
> +const char *CleanQuerytext(const char *query, int *location, int *len);
>  JumbleState *JumbleQuery(Query *query, const char *querytext);

I think pushing in more than one commit is a reasonable approach if they
are well-contained, but if you do that it'd be better to avoid
introducing a function with one name and renaming it in your next
commit.

-- 
Álvaro Herrera   Valdivia, Chile
"Just treat us the way you want to be treated + some extra allowance
 for ignorance."(Michael Brusser)

Re: PoC/WIP: Extended statistics on expressions

[Justin Pryzby]

I got this crash running sqlsmith:

#1  0x7f907574b801 in __GI_abort () at abort.c:79
#2  0x5646b95a35f8 in ExceptionalCondition 
(conditionName=conditionName@entry=0x5646b97411db "bms_num_members(varnos) == 
1", errorType=errorType@entry=0x5646b95fa00b "FailedAssertion", 
fileName=fileName@entry=0x5646b9739dbe "selfuncs.c", 
lineNumber=lineNumber@entry=3332) at assert.c:69
#3  0x5646b955c9a1 in add_unique_group_expr (vars=0x5646bbd9e200, 
expr=0x5646b9eb0c30, exprinfos=0x5646bbd9e100, root=0x5646ba9a0cb0) at 
selfuncs.c:3332
#4  add_unique_group_expr (root=0x5646ba9a0cb0, exprinfos=0x5646bbd9e100, 
expr=0x5646b9eb0c30, vars=0x5646bbd9e200) at selfuncs.c:3307
#5  0x5646b955d560 in estimate_num_groups () at selfuncs.c:3558
#6  0x5646b93ad004 in create_distinct_paths (input_rel=, 
root=0x5646ba9a0cb0) at planner.c:4808
#7  grouping_planner () at planner.c:2238
#8  0x5646b93ae0ef in subquery_planner (glob=glob@entry=0x5646ba9a0b98, 
parse=parse@entry=0x5646ba905d80, parent_root=parent_root@entry=0x0, 
hasRecursion=hasRecursion@entry=false, tuple_fraction=tuple_fraction@entry=0)
at planner.c:1024
#9  0x5646b93af543 in standard_planner (parse=0x5646ba905d80, 
query_string=, cursorOptions=256, boundParams=) 
at planner.c:404
#10 0x5646b94873ac in pg_plan_query (querytree=0x5646ba905d80, 
query_string=0x5646b9cd87e0 "select distinct \n  \n
pg_catalog.variance(\n  
cast(pg_catalog.pg_stat_get_bgwriter_timed_checkpoints() as int8)) over 
(partition by subq_0.c2 order by subq_0.c0) as c0, \n  subq_0.c2 as c1, \n  
sub"..., cursorOptions=256, boundParams=0x0) at postgres.c:821
#11 0x5646b94874a1 in pg_plan_queries (querytrees=0x5646baaba250, 
query_string=query_string@entry=0x5646b9cd87e0 "select distinct \n  \n
pg_catalog.variance(\n  
cast(pg_catalog.pg_stat_get_bgwriter_timed_checkpoints() as int8)) over 
(partition by subq_0.c2 order by subq_0.c0) as c0, \n  subq_0.c2 as c1, \n  
sub"..., cursorOptions=cursorOptions@entry=256, 
boundParams=boundParams@entry=0x0) at postgres.c:912
#12 0x5646b9487888 in exec_simple_query () at postgres.c:1104

2021-03-24 03:06:12.489 CDT postmaster[11653] LOG:  server process (PID 11696) 
was terminated by signal 6: Aborted
2021-03-24 03:06:12.489 CDT postmaster[11653] DETAIL:  Failed process was 
running: select distinct 
  
pg_catalog.variance(
  cast(pg_catalog.pg_stat_get_bgwriter_timed_checkpoints() as 
int8)) over (partition by subq_0.c2 order by subq_0.c0) as c0, 
  subq_0.c2 as c1, 
  subq_0.c0 as c2, 
  subq_0.c2 as c3, 
  subq_0.c1 as c4, 
  subq_0.c1 as c5, 
  subq_0.c0 as c6
from 
  (select  
ref_1.foreign_server_catalog as c0, 
ref_1.authorization_identifier as c1, 
sample_2.tgname as c2, 
ref_1.foreign_server_catalog as c3
  from 
pg_catalog.pg_stat_database_conflicts as ref_0
left join information_schema._pg_user_mappings as ref_1
on (ref_0.datname < ref_0.datname)
  inner join pg_catalog.pg_amproc as sample_0 tablesample 
system (5) 
  on (cast(null as uuid) < cast(null as uuid))
left join pg_catalog.pg_aggregate as sample_1 tablesample 
system (2.9) 
on (sample_0.amprocnum = sample_1.aggnumdirectargs )
  inner join pg_catalog.pg_trigger as sample_2 tablesampl

Re: [HACKERS] Custom compression methods

[Dilip Kumar]

On Wed, Mar 24, 2021 at 1:22 PM Jaime Casanova
 wrote:
>
> On Fri, Mar 19, 2021 at 2:44 PM Robert Haas  wrote:
> >
> > I committed the core patch (0003) with a bit more editing.  Let's see
> > what the buildfarm thinks.
> >
>
> I think this is bbe0a81db69bd10bd166907c3701492a29aca294, right?
> This introduced a new assert failure, steps to reproduce:
>
> """
> create table t1 (col1 text, col2 text);
> create unique index on t1 ((col1 || col2));
> insert into t1 values((select array_agg(md5(g::text))::text from
> generate_series(1, 256) g), version());
> """
>
> Attached is a backtrace from current HEAD

Thanks for reporting this issue.  Actually, I missed setting the
attcompression for the expression index and that is causing this
assert.  I will send a patch in some time.

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

RE: Disable WAL logging to speed up data loading

[tsunakawa.ta...@fujitsu.com]

From: Stephen Frost 
> * tsunakawa.ta...@fujitsu.com (tsunakawa.ta...@fujitsu.com) wrote:
> > As Laurenz-san kindly replied, the database server refuses to start with a
> clear message.  So, it's similarly very clear what happens.  The user will 
> never
> unknowingly resume operation with possibly corrupt data.
> 
> No, instead they'll just have a completely broken system that has to be 
> rebuilt or
> restored from a backup.  That doesn't strike me as a good result.

Your understanding is correct.  What I wanted to answer to confirm is that the 
behavior is clear: the server refuses to start, and the user knows he/she 
should restore the database from backup.


> > So, I understood the point boils down to elegance.  Could I ask what makes
> you feel ALTER TABLE UNLOGGED/LOGGED is (more) elegant?  I'm purely
> asking as a user.
> 
> The impact is localized to those specific tables.  The rest of the system 
> should
> come up cleanly and there won't be corruption, instead merely the lack of data
> in UNLOGGED tables.

So, I took your point as the ease and fast time of restore after a crash: the 
user just has to restore the lost table data using COPY FROM from files that 
was saved before the data loading job using COPY TO.

In that sense, the backup and restoration of the whole database is an option 
for users when they have some instant backup and restore feature available.


> > (I don't want to digress, but if we consider the number of options for
> > wal_level as an issue, I feel it's not elegant to have separate
> > "replica" and "logical".)
> 
> Do you know of a way to avoid having those two distinct levels while still 
> writing
> only the WAL needed depending on if a system is doing logical replication or
> not..?  If you've got suggestions on how to eliminate one of those levels, I'm
> sure there would be interest in doing so.  I don't see the fact that we have
> those two levels as justification for adding another spelling of 'minimal'.

Sorry, I have almost no knowledge of logical replication implementation.  So, 
being ignorant of its intricacies, I have felt like as a user "Why do I have to 
set wal_level = logical, because streaming replication and logical replication 
are both replication features?  If the implementation needs some additional WAL 
for logical replication, why doesn't the server automatically emit the WAL when 
the target table of DML statements is in a publication?"


> > The elegance of wal_level = none is that the user doesn't have to
> > remember to add ALTER TABLE to the data loading job when they add load
> > target tables/partitions.  If they build and use their own (shell)
> > scripts to load data, that won't be burdon or forgotten.  But what
> > would they have to do when they use ETL tools like Talend, Pentaho,
> > and Informatica Power Center?  Do those tools allow users to add
> > custom processing like ALTER TABLE to the data loading job steps for
> > each table?  (AFAIK, not.)
> 
> I don't buy the argument that having to 'remember' to do an ALTER TABLE is
> such a burden when it means that the database will still be consistent and
> operational after a crash.

That depends on whether an instant backup and restore feature is at hand.  If 
the user is comfortable with it, wal_level = none is easier and more 
attractive.  At least, I don't want the feature to be denied.


> As for data loading tools, surely they support loading data into UNLOGGED
> tables and it's certainly not hard to have a script run around and flip those
> tables to LOGGED after they're loaded, and I do actually believe some of those
> tools support building processes of which one step could be such a command
> (I'm fairly confident Pentaho, in particular, does as I remember building such
> pipelines myself...).

Oh, Pentaho has such a feature, doesn't it?  But isn't it a separate step from 
the data output step?  Here, I assume ETL tools allow users to compose a data 
loading job from multiple steps: data input, transformation, data output, etc.  
I guess the user can't directly incorporate ALTER TABLE into the data output 
step, and has to add separate custom steps for ALTER TABLE.  That's burdonsome 
and forgettable, I think.


Regards
TakayukiTsunakawa

Re: shared memory stats: high level design decisions: consistency, dropping

[Alvaro Herrera]

On 2021-Mar-21, Andres Freund wrote:

> We currently also fetch the full stats in places like autovacuum.c. Where we
> don't need repeated access to be consistent - we even explicitly force the
> stats to be re-read for every single table that's getting vacuumed.
> 
> Even if we to just cache already accessed stats, places like do_autovacuum()
> would end up with a completely unnecessary cache of all tables, blowing up
> memory usage by a large amount on systems with lots of relations.

It's certainly not the case that autovacuum needs to keep fully
consistent stats.  That's just the way that seemed easier (?) to do at
the time.  Unless I misunderstand things severely, we could just have
autovacuum grab all necessary numbers for one database at the start of a
run, not cache anything, then re-read the numbers for one table as it
rechecks that table.

Resetting before re-reading was obviously necessary because the
built-in snapshotting made it impossible to freshen up the numbers at
the recheck step.

-- 
Álvaro Herrera39°49'30"S 73°17'W

Re: Feature improvement: can we add queryId for pg_catalog.pg_stat_activity view?

[Julien Rouhaud]

On Wed, Mar 24, 2021 at 05:12:35AM -0300, Alvaro Herrera wrote:
> On 2021-Mar-22, Bruce Momjian wrote:
> 
> > diff --git a/src/include/catalog/pg_proc.dat 
> > b/src/include/catalog/pg_proc.dat
> > index e259531f60..9550de0798 100644
> > --- a/src/include/catalog/pg_proc.dat
> > +++ b/src/include/catalog/pg_proc.dat
> > @@ -5249,9 +5249,9 @@
> >proname => 'pg_stat_get_activity', prorows => '100', proisstrict => 'f',
> >proretset => 't', provolatile => 's', proparallel => 'r',
> >prorettype => 'record', proargtypes => 'int4',
> > -  proallargtypes => 
> > '{int4,oid,int4,oid,text,text,text,text,text,timestamptz,timestamptz,timestamptz,timestamptz,inet,text,int4,xid,xid,text,bool,text,text,int4,text,numeric,text,bool,text,bool,int4}',
> > -  proargmodes => 
> > '{i,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o}',
> > -  proargnames => 
> > '{pid,datid,pid,usesysid,application_name,state,query,wait_event_type,wait_event,xact_start,query_start,backend_start,state_change,client_addr,client_hostname,client_port,backend_xid,backend_xmin,backend_type,ssl,sslversion,sslcipher,sslbits,ssl_client_dn,ssl_client_serial,ssl_issuer_dn,gss_auth,gss_princ,gss_enc,leader_pid}',
> > +  proallargtypes => 
> > '{int4,oid,int4,oid,text,text,text,text,text,timestamptz,timestamptz,timestamptz,timestamptz,inet,text,int4,xid,xid,text,bool,text,text,int4,text,numeric,text,bool,text,bool,int4,int8}',
> > +  proargmodes => 
> > '{i,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o}',
> > +  proargnames => 
> > '{pid,datid,pid,usesysid,application_name,state,query,wait_event_type,wait_event,xact_start,query_start,backend_start,state_change,client_addr,client_hostname,client_port,backend_xid,backend_xmin,backend_type,ssl,sslversion,sslcipher,sslbits,ssl_client_dn,ssl_client_serial,ssl_issuer_dn,gss_auth,gss_princ,gss_enc,leader_pid,queryid}',
> 
> BTW why do you put the queryid column at the end of the column list
> here?  It seems awkward.  Can we put it perhaps between state and query?

I thought that it would be better to have it at the end as it can always be
NULL (and will be by default), which I guess was also the reason to have
leader_pid there.  I'm all in favor to have queryid near the query, and
while at it leader_pid near the pid.

> > -const char *clean_querytext(const char *query, int *location, int *len);
> > +const char *CleanQuerytext(const char *query, int *location, int *len);
> >  JumbleState *JumbleQuery(Query *query, const char *querytext);
> 
> I think pushing in more than one commit is a reasonable approach if they
> are well-contained

They should, as I incrementally built on top of the first one.  I also just
double checked the patchset and each new commit compiles and passes the
regression tests.

> but if you do that it'd be better to avoid
> introducing a function with one name and renaming it in your next
> commit.

Oops, I apparently messed a fixup when working on it.  Bruce, should I take
care of that of do you want to?  I think you have some local modifications
already I'd rather not miss some changes.

Re: PoC/WIP: Extended statistics on expressions

[Tomas Vondra]

Hi Justin,

Unfortunately the query is incomplete, so I can't quite determine what
went wrong. Can you extract the full query causing the crash, either
from the server log or from a core file?

thanks

On 3/24/21 9:14 AM, Justin Pryzby wrote:
> I got this crash running sqlsmith:
> 
> #1  0x7f907574b801 in __GI_abort () at abort.c:79
> #2  0x5646b95a35f8 in ExceptionalCondition 
> (conditionName=conditionName@entry=0x5646b97411db "bms_num_members(varnos) == 
> 1", errorType=errorType@entry=0x5646b95fa00b "FailedAssertion", 
> fileName=fileName@entry=0x5646b9739dbe "selfuncs.c", 
> lineNumber=lineNumber@entry=3332) at assert.c:69
> #3  0x5646b955c9a1 in add_unique_group_expr (vars=0x5646bbd9e200, 
> expr=0x5646b9eb0c30, exprinfos=0x5646bbd9e100, root=0x5646ba9a0cb0) at 
> selfuncs.c:3332
> #4  add_unique_group_expr (root=0x5646ba9a0cb0, exprinfos=0x5646bbd9e100, 
> expr=0x5646b9eb0c30, vars=0x5646bbd9e200) at selfuncs.c:3307
> #5  0x5646b955d560 in estimate_num_groups () at selfuncs.c:3558
> #6  0x5646b93ad004 in create_distinct_paths (input_rel=, 
> root=0x5646ba9a0cb0) at planner.c:4808
> #7  grouping_planner () at planner.c:2238
> #8  0x5646b93ae0ef in subquery_planner (glob=glob@entry=0x5646ba9a0b98, 
> parse=parse@entry=0x5646ba905d80, parent_root=parent_root@entry=0x0, 
> hasRecursion=hasRecursion@entry=false, tuple_fraction=tuple_fraction@entry=0)
> at planner.c:1024
> #9  0x5646b93af543 in standard_planner (parse=0x5646ba905d80, 
> query_string=, cursorOptions=256, boundParams=) 
> at planner.c:404
> #10 0x5646b94873ac in pg_plan_query (querytree=0x5646ba905d80, 
> query_string=0x5646b9cd87e0 "select distinct \n  \n
> pg_catalog.variance(\n  
> cast(pg_catalog.pg_stat_get_bgwriter_timed_checkpoints() as int8)) over 
> (partition by subq_0.c2 order by subq_0.c0) as c0, \n  subq_0.c2 as c1, \n  
> sub"..., cursorOptions=256, boundParams=0x0) at postgres.c:821
> #11 0x5646b94874a1 in pg_plan_queries (querytrees=0x5646baaba250, 
> query_string=query_string@entry=0x5646b9cd87e0 "select distinct \n  \n
> pg_catalog.variance(\n  
> cast(pg_catalog.pg_stat_get_bgwriter_timed_checkpoints() as int8)) over 
> (partition by subq_0.c2 order by subq_0.c0) as c0, \n  subq_0.c2 as c1, \n  
> sub"..., cursorOptions=cursorOptions@entry=256, 
> boundParams=boundParams@entry=0x0) at postgres.c:912
> #12 0x5646b9487888 in exec_simple_query () at postgres.c:1104
> 
> 2021-03-24 03:06:12.489 CDT postmaster[11653] LOG:  server process (PID 
> 11696) was terminated by signal 6: Aborted
> 2021-03-24 03:06:12.489 CDT postmaster[11653] DETAIL:  Failed process was 
> running: select distinct 
>   
> pg_catalog.variance(
>   cast(pg_catalog.pg_stat_get_bgwriter_timed_checkpoints() as 
> int8)) over (partition by subq_0.c2 order by subq_0.c0) as c0, 
>   subq_0.c2 as c1, 
>   subq_0.c0 as c2, 
>   subq_0.c2 as c3, 
>   subq_0.c1 as c4, 
>   subq_0.c1 as c5, 
>   subq_0.c0 as c6
> from 
>   (select  
> ref_1.foreign_server_catalog as c0, 
> ref_1.authorization_identifier as c1, 
> sample_2.tgname as c2, 
> ref_1.foreign_server_catalog as c3
>   from 
> pg_catalog.pg_stat_database_conflicts as ref_0
> left join information_schema._pg_user_mappings as 
> ref_1
> on (ref_0.datname < ref_0.datname)
>   inner join pg_catalog.pg_amproc as sample_0 tablesample 
> system (5) 
>   on (cast(null as uuid) < cast(null as uuid))
> left join pg_catalog.pg_aggregate as sample_1 tablesample 
> system (2.9) 
> on (sample_0.amprocnum = sample_1.aggnumdirectargs )
>   inner join pg_catalog.pg_trigger as sample_2 tablesampl
> 

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: [HACKERS] Custom compression methods

[Dilip Kumar]

On Wed, Mar 24, 2021 at 1:43 PM Dilip Kumar  wrote:

> >
> > """
> > create table t1 (col1 text, col2 text);
> > create unique index on t1 ((col1 || col2));
> > insert into t1 values((select array_agg(md5(g::text))::text from
> > generate_series(1, 256) g), version());
> > """
> >
> > Attached is a backtrace from current HEAD
>
> Thanks for reporting this issue.  Actually, I missed setting the
> attcompression for the expression index and that is causing this
> assert.  I will send a patch in some time.

PFA, patch to fix the issue.

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com


v1-0001-Fix-attcompression-for-index-expression-columns.patch
Description: Binary data

Re: PoC/WIP: Extended statistics on expressions

[Justin Pryzby]

On Wed, Mar 24, 2021 at 09:54:22AM +0100, Tomas Vondra wrote:
> Hi Justin,
> 
> Unfortunately the query is incomplete, so I can't quite determine what
> went wrong. Can you extract the full query causing the crash, either
> from the server log or from a core file?

Oh, shoot, I didn't realize it was truncated, and I already destroyed the core
and moved on to something else...

But this fails well enough, and may be much shorter than the original :)

select distinct
 pg_catalog.variance(
   cast(pg_catalog.pg_stat_get_bgwriter_timed_checkpoints() as int8)) over 
(partition by subq_0.c2 order by subq_0.c0) as c0,
   subq_0.c2 as c1, subq_0.c0 as c2, subq_0.c2 as c3, subq_0.c1 as c4, 
subq_0.c1 as c5, subq_0.c0 as c6
 from
   (select
 ref_1.foreign_server_catalog as c0,
 ref_1.authorization_identifier as c1,
 sample_2.tgname as c2,
 ref_1.foreign_server_catalog as c3
   from
 pg_catalog.pg_stat_database_conflicts as ref_0
 left join information_schema._pg_user_mappings as ref_1
 on (ref_0.datname < ref_0.datname)
   inner join pg_catalog.pg_amproc as sample_0 tablesample system 
(5)
   on (cast(null as uuid) < cast(null as uuid))
 left join pg_catalog.pg_aggregate as sample_1 tablesample system 
(2.9)
 on (sample_0.amprocnum = sample_1.aggnumdirectargs )
   inner join pg_catalog.pg_trigger as sample_2 tablesample system (1) 
on true )subq_0;

TRAP: FailedAssertion("bms_num_members(varnos) == 1", File: "selfuncs.c", Line: 
3332, PID: 16422)

Also ... with this patch CREATE STATISTIC is no longer rejecting multiple
tables, and instead does this:

postgres=# CREATE STATISTICS xt ON a FROM t JOIN t ON true;
ERROR:  schema "i" does not exist

-- 
Justin

Re: [HACKERS] Custom compression methods

[Justin Pryzby]

On Wed, Mar 24, 2021 at 02:24:41PM +0530, Dilip Kumar wrote:
> On Wed, Mar 24, 2021 at 1:43 PM Dilip Kumar  wrote:
> > > create table t1 (col1 text, col2 text);
> > > create unique index on t1 ((col1 || col2));
> > > insert into t1 values((select array_agg(md5(g::text))::text from
> > > generate_series(1, 256) g), version());
> > >
> > > Attached is a backtrace from current HEAD
> >
> > Thanks for reporting this issue.  Actually, I missed setting the
> > attcompression for the expression index and that is causing this
> > assert.  I will send a patch in some time.
> 
> PFA, patch to fix the issue.

Could you include a test case exercizing this code path ?
Like Jaime's reproducer.

-- 
Justin

Re: make the stats collector shutdown without writing the statsfiles if the immediate shutdown is requested.

[Fujii Masao]

On 2021/03/23 15:50, Fujii Masao wrote:

+ * The statistics collector is started by the postmaster as soon as the
+ * startup subprocess finishes.

This comment needs to be updated? Because the stats collector can
be invoked when the startup process sends PMSIGNAL_BEGIN_HOT_STANDBY
signal.


I updated this comment in the patch.
Attached is the updated version of the patch.

Barring any objection, I'm thinking to commit this patch.

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
diff --git a/src/backend/postmaster/interrupt.c 
b/src/backend/postmaster/interrupt.c
index dd9136a942..d1b1f95400 100644
--- a/src/backend/postmaster/interrupt.c
+++ b/src/backend/postmaster/interrupt.c
@@ -64,9 +64,28 @@ SignalHandlerForConfigReload(SIGNAL_ARGS)
 }
 
 /*
- * Simple signal handler for exiting quickly as if due to a crash.
+ * Simple signal handler for processes which have not yet touched or do not
+ * touch shared memory to exit quickly.
  *
- * Normally, this would be used for handling SIGQUIT.
+ * Note that if processes already touched shared memory, use
+ * SignalHandlerForCrashExit() instead and force the postmaster into
+ * a system reset cycle because shared memory may be corrupted.
+ */
+void
+SignalHandlerForNonCrashExit(SIGNAL_ARGS)
+{
+   /*
+* Since we don't touch shared memory, we can just pull the plug and 
exit
+* without running any atexit handlers.
+*/
+   _exit(1);
+}
+
+/*
+ * Simple signal handler for processes which have touched shared memory to
+ * exit quickly.
+ *
+ * Normally, this would be used for handling SIGQUIT as if due to a crash.
  */
 void
 SignalHandlerForCrashExit(SIGNAL_ARGS)
@@ -93,9 +112,8 @@ SignalHandlerForCrashExit(SIGNAL_ARGS)
  * shut down and exit.
  *
  * Typically, this handler would be used for SIGTERM, but some processes use
- * other signals. In particular, the checkpointer exits on SIGUSR2, the
- * stats collector on SIGQUIT, and the WAL writer exits on either SIGINT
- * or SIGTERM.
+ * other signals. In particular, the checkpointer and the stats collector exit
+ * on SIGUSR2, and the WAL writer exits on either SIGINT or SIGTERM.
  *
  * ShutdownRequestPending should be checked at a convenient place within the
  * main loop, or else the main loop should call HandleMainLoopInterrupts.
diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
index 60f45ccc4e..fd0af0f289 100644
--- a/src/backend/postmaster/pgstat.c
+++ b/src/backend/postmaster/pgstat.c
@@ -1,7 +1,23 @@
 /* --
  * pgstat.c
  *
- * All the statistics collector stuff hacked up in one big, ugly file.
+ * All the statistics collector stuff hacked up in one big, ugly file.
+ *
+ * The statistics collector is started by the postmaster as soon as the
+ * startup subprocess finishes, or as soon as the postmaster is ready
+ * to accept read only connections during archive recovery.  It remains
+ * alive until the postmaster commands it to terminate.  Normal
+ * termination is by SIGUSR2 after the checkpointer must exit(0),
+ * which instructs the statistics collector to save the final statistics
+ * to reuse at next startup and then exit(0).
+ * Emergency termination is by SIGQUIT; like any backend, the statistics
+ * collector will exit quickly without saving the final statistics.  It's
+ * ok because the startup process will remove all statistics at next
+ * startup after emergency termination.
+ *
+ * Because the statistics collector doesn't touch shared memory, even if
+ * the statistics collector exits unexpectedly, the postmaster doesn't
+ * treat it as a crash.  The postmaster will just try to restart a new one.
  *
  * TODO:   - Separate collector, postmaster and backend stuff
  *   into different files.
@@ -724,6 +740,7 @@ pgstat_reset_remove_files(const char *directory)
 
snprintf(fname, sizeof(fname), "%s/%s", directory,
 entry->d_name);
+   elog(DEBUG2, "removing stats file \"%s\"", fname);
unlink(fname);
}
FreeDir(dir);
@@ -4821,17 +4838,31 @@ PgstatCollectorMain(int argc, char *argv[])
 
/*
 * Ignore all signals usually bound to some action in the postmaster,
-* except SIGHUP and SIGQUIT.  Note we don't need a SIGUSR1 handler to
-* support latch operations, because we only use a local latch.
+* except SIGHUP, SIGQUIT and SIGUSR2.  Note we don't need a SIGUSR1
+* handler to support latch operations, because we only use a local 
latch.
+*
+* We deliberately ignore SIGTERM and exit in correct order because we
+* want to collect the stats sent during the shutdown from all 
processes.
+* SIGTERM will be received during a standard Unix system shutdown cycle
+* because init will SIGTERM all processes at once, and the postmaster
+* will SIGTERM all processes

Re: make the stats collector shutdown without writing the statsfiles if the immediate shutdown is requested.

[Fujii Masao]

On 2021/03/24 3:51, Andres Freund wrote:

Hi,

On 2021-03-23 15:50:46 +0900, Fujii Masao wrote:

This fact makes me wonder that if we collect the statistics about WAL writing
from walreceiver as we discussed in other thread, the stats collector should
be invoked at more earlier stage. IIUC walreceiver can be invoked before
PMSIGNAL_BEGIN_HOT_STANDBY is sent.


FWIW, in the shared memory stats patch the stats subsystem is
initialized early on by the startup process.


This is good news!

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

Re: [HACKERS] Custom compression methods

[Dilip Kumar]

On Wed, Mar 24, 2021 at 2:49 PM Justin Pryzby  wrote:
>
> On Wed, Mar 24, 2021 at 02:24:41PM +0530, Dilip Kumar wrote:
> > On Wed, Mar 24, 2021 at 1:43 PM Dilip Kumar  wrote:
> > > > create table t1 (col1 text, col2 text);
> > > > create unique index on t1 ((col1 || col2));
> > > > insert into t1 values((select array_agg(md5(g::text))::text from
> > > > generate_series(1, 256) g), version());
> > > >
> > > > Attached is a backtrace from current HEAD
> > >
> > > Thanks for reporting this issue.  Actually, I missed setting the
> > > attcompression for the expression index and that is causing this
> > > assert.  I will send a patch in some time.
> >
> > PFA, patch to fix the issue.
>
> Could you include a test case exercizing this code path ?
> Like Jaime's reproducer.

I will do that.


-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

Re: Nicer error when connecting to standby with hot_standby=off

[Fujii Masao]

On 2021/03/24 16:59, Alvaro Herrera wrote:

On 2021-Mar-24, Fujii Masao wrote:


On 2021/03/24 5:59, Tom Lane wrote:

Alvaro Herrera  writes:

FATAL:  the database system is starting up
DETAIL:  WAL is being applied to recover from a system crash.
or
DETAIL:  The system is applying WAL to recover from a system crash.
or
DETAIL:  The startup process is applying WAL to recover from a system crash.


I don't think the postmaster has enough context to know if that's
actually true.  It just launches the startup process and waits for
results.  If somebody saw this during a normal (non-crash) startup,
they'd be justifiably alarmed.


Yes, so logging "the database system is starting up" seems enough to me.


No objection.


Thanks! So I changed the message reported at PM_STARTUP to that one,
based on v8 patch that James posted upthread. I also ran pgindent for
the patch. Attached is the updated version of the patch.

Barring any objection, I will commit this.

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
diff --git a/src/backend/postmaster/postmaster.c 
b/src/backend/postmaster/postmaster.c
index ef0be4ca38..4a3ca78c1b 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -2294,6 +2294,18 @@ retry1:
(errcode(ERRCODE_CANNOT_CONNECT_NOW),
 errmsg("the database system is 
starting up")));
break;
+   case CAC_NOTCONSISTENT:
+   if (EnableHotStandby)
+   ereport(FATAL,
+   
(errcode(ERRCODE_CANNOT_CONNECT_NOW),
+errmsg("the database system is 
not yet accepting connections"),
+errdetail("Consistent recovery 
state has not been yet reached.")));
+   else
+   ereport(FATAL,
+   
(errcode(ERRCODE_CANNOT_CONNECT_NOW),
+errmsg("the database system is 
not accepting connections"),
+errdetail("Hot standby mode is 
disabled.")));
+   break;
case CAC_SHUTDOWN:
ereport(FATAL,
(errcode(ERRCODE_CANNOT_CONNECT_NOW),
@@ -2435,10 +2447,11 @@ canAcceptConnections(int backend_type)
{
if (Shutdown > NoShutdown)
return CAC_SHUTDOWN;/* shutdown is pending */
-   else if (!FatalError &&
-(pmState == PM_STARTUP ||
- pmState == PM_RECOVERY))
+   else if (!FatalError && pmState == PM_STARTUP)
return CAC_STARTUP; /* normal startup */
+   else if (!FatalError && pmState == PM_RECOVERY)
+   return CAC_NOTCONSISTENT;   /* not yet at 
consistent recovery
+   
 * state */
else
return CAC_RECOVERY;/* else must be crash recovery 
*/
}
diff --git a/src/include/libpq/libpq-be.h b/src/include/libpq/libpq-be.h
index 30fb4e613d..891394b0c3 100644
--- a/src/include/libpq/libpq-be.h
+++ b/src/include/libpq/libpq-be.h
@@ -70,7 +70,12 @@ typedef struct
 
 typedef enum CAC_state
 {
-   CAC_OK, CAC_STARTUP, CAC_SHUTDOWN, CAC_RECOVERY, CAC_TOOMANY,
+   CAC_OK,
+   CAC_STARTUP,
+   CAC_SHUTDOWN,
+   CAC_RECOVERY,
+   CAC_NOTCONSISTENT,
+   CAC_TOOMANY,
CAC_SUPERUSER
 } CAC_state;
 

Re: pgsql: Move tablespace path re-creation from the makefiles to pg_regres

[Christoph Berg]

Re: Michael Paquier
> So you basically mimicked the makefile rule that this commit removed
> into your own test suite.  Reverting the change does not really help,
> because we'd be back to square one where there would be problems in
> parallel runs for developers.  Saying that, I would not mind adding an
> option to pg_regress to control if this cleanup code is triggered or
> not, say something like --no-tablespace-cleanup?  Then, you could just
> pass down the option by yourself before creating your tablespace path
> as you wish.

I don't think adding more snowflake code just for this use case makes
sense, so I can stick to my workaround.

I just wanted to point out that the only thing preventing the core
testsuite from being run as a true client app is this tablespace
thing, which might be a worthwhile fix on its own.

Maybe creating the tablespace directory from within the testsuite
would suffice?

CREATE TABLE foo (t text);
COPY foo FROM PROGRAM 'mkdir @testtablespace@';
CREATE TABLESPACE regress_tblspace LOCATION '@testtablespace@';

Christoph

Re: Replication slot stats misgivings

[Amit Kapila]

On Tue, Mar 23, 2021 at 10:54 PM Andres Freund  wrote:
>
> On 2021-03-23 23:37:14 +0900, Masahiko Sawada wrote:
>
> > > > Maybe we can compare the slot name in the
> > > > received message to the name in the element of replSlotStats. If they
> > > > don’t match, we swap entries in replSlotStats to synchronize the index
> > > > of the replication slot in ReplicationSlotCtl->replication_slots and
> > > > replSlotStats. If we cannot find the entry in replSlotStats that has
> > > > the name in the received message, it probably means either it's a new
> > > > slot or the previous create message is dropped, we can create the new
> > > > stats for the slot. Is that what you mean, Andres?
>
> That doesn't seem great. Slot names are imo a poor identifier for
> something happening asynchronously. The stats collector regularly
> doesn't process incoming messages for periods of time because it is busy
> writing out the stats file. That's also when messages to it are most
> likely to be dropped (likely because the incoming buffer is full).
>

Leaving aside restart case, without some sort of such sanity checking,
if both drop (of old slot) and create (of new slot) messages are lost
then we will start accumulating stats in old slots. However, if only
one of them is lost then there won't be any such problem.

> Perhaps we could have RestoreSlotFromDisk() send something to the stats
> collector ensuring the mapping makes sense?
>

Say if we send just the index location of each slot then probably we
can setup replSlotStats. Now say before the restart if one of the drop
messages was missed (by stats collector) and that happens to be at
some middle location, then we would end up restoring some already
dropped slot, leaving some of the still required ones. However, if
there is some sanity identifier like name along with the index, then I
think that would have worked for such a case.

I think it would have been easier if we would have some OID type of
identifier for each slot. But, without that may be index location of
ReplicationSlotCtl->replication_slots and slotname combination can
reduce the chances of slot stats go wrong quite less even if not zero.
If not name, do we have anything else in a slot that can be used for
some sort of sanity checking?

-- 
With Regards,
Amit Kapila.

Re: [HACKERS] Custom compression methods

[Dilip Kumar]

On Wed, Mar 24, 2021 at 3:10 PM Dilip Kumar  wrote:
>
> On Wed, Mar 24, 2021 at 2:49 PM Justin Pryzby  wrote:
> >
> > On Wed, Mar 24, 2021 at 02:24:41PM +0530, Dilip Kumar wrote:
> > > On Wed, Mar 24, 2021 at 1:43 PM Dilip Kumar  wrote:
> > > > > create table t1 (col1 text, col2 text);
> > > > > create unique index on t1 ((col1 || col2));
> > > > > insert into t1 values((select array_agg(md5(g::text))::text from
> > > > > generate_series(1, 256) g), version());
> > > > >
> > > > > Attached is a backtrace from current HEAD
> > > >
> > > > Thanks for reporting this issue.  Actually, I missed setting the
> > > > attcompression for the expression index and that is causing this
> > > > assert.  I will send a patch in some time.
> > >
> > > PFA, patch to fix the issue.
> >
> > Could you include a test case exercizing this code path ?
> > Like Jaime's reproducer.
>
> I will do that.

0001 ->shows compression method for the index attribute in index describe
0002 -> fix the reported bug (test case included)

Apart from this, I was thinking that currently, we are allowing to
ALTER SET COMPRESSION only for the table and matview,  IMHO it makes
sense to allow to alter the compression method for the index column as
well?  I mean it is just a one-line change, but just wanted to know
the opinion from others.  It is not required for the storage because
indexes can not have a toast table but index attributes can be
compressed so it makes sense to allow to alter the compression method.
Thought?


--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
From fa3ee05e21af64af86bedcc919488300c4f9a92d Mon Sep 17 00:00:00 2001
From: Dilip Kumar 
Date: Wed, 24 Mar 2021 15:08:14 +0530
Subject: [PATCH v2 1/2] Show compression method in index describe

---
 src/bin/psql/describe.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index eeac0ef..9d15324 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -1897,6 +1897,7 @@ describeOneTableDetails(const char *schemaname,
 		if (pset.sversion >= 14 &&
 			!pset.hide_compression &&
 			(tableinfo.relkind == RELKIND_RELATION ||
+			 tableinfo.relkind == RELKIND_INDEX ||
 			 tableinfo.relkind == RELKIND_PARTITIONED_TABLE ||
 			 tableinfo.relkind == RELKIND_MATVIEW))
 		{
-- 
1.8.3.1

From 54edcec2d44cfac01e1c2d20cc413dac57496ca9 Mon Sep 17 00:00:00 2001
From: Dilip Kumar 
Date: Wed, 24 Mar 2021 14:02:06 +0530
Subject: [PATCH v2 2/2] Fix attcompression for index expression columns

For expression columns the attcompression was not set.  So this patch
set it to the default compression method for compressible types otherwise
to the invalid compression method.
---
 src/backend/catalog/index.c | 11 +++
 src/test/regress/expected/compression.out   | 13 +
 src/test/regress/expected/compression_1.out | 14 ++
 src/test/regress/sql/compression.sql|  8 
 4 files changed, 46 insertions(+)

diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 397d70d..b5a79ce 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -30,6 +30,7 @@
 #include "access/relscan.h"
 #include "access/sysattr.h"
 #include "access/tableam.h"
+#include "access/toast_compression.h"
 #include "access/transam.h"
 #include "access/visibilitymap.h"
 #include "access/xact.h"
@@ -379,6 +380,16 @@ ConstructTupleDescriptor(Relation heapRelation,
 			to->attalign = typeTup->typalign;
 			to->atttypmod = exprTypmod(indexkey);
 
+			/*
+			 * For expression column, if attribute type storage is compressible
+			 * then set the default compression method, otherwise invalid
+			 * compression method.
+			 */
+			if (IsStorageCompressible(typeTup->typstorage))
+to->attcompression = GetDefaultToastCompression();
+			else
+to->attcompression = InvalidCompressionMethod;
+
 			ReleaseSysCache(tuple);
 
 			/*
diff --git a/src/test/regress/expected/compression.out b/src/test/regress/expected/compression.out
index c2f2e0e..19707fb 100644
--- a/src/test/regress/expected/compression.out
+++ b/src/test/regress/expected/compression.out
@@ -313,6 +313,19 @@ SELECT pg_column_compression(f1) FROM cmdata;
  lz4
 (2 rows)
 
+-- test expression index
+DROP TABLE cmdata2;
+CREATE TABLE cmdata2 (f1 TEXT COMPRESSION pglz, f2 TEXT COMPRESSION lz4);
+CREATE UNIQUE INDEX idx1 ON cmdata2 ((f1 || f2));
+INSERT INTO cmdata2 VALUES((SELECT array_agg(md5(g::TEXT))::TEXT FROM
+generate_series(1, 50) g), VERSION());
+\d+ idx1
+Index "public.idx1"
+ Column | Type | Key? | Definition | Storage  | Compression | Stats target 
++--+--++--+-+--
+ expr   | text | yes  | (f1 || f2) | extended | pglz| 
+unique, btree, for table "public.cmdata2"
+
 -- check data is ok
 SELECT length(f1) FROM cmdata;
  length 
diff --git a/src/test/regress/expected/compression_1.out b/src/test/regress/expect

RE: [HACKERS] logical decoding of two-phase transactions

[houzj.f...@fujitsu.com]

> I have incorporated all your changes and additionally made few more changes
> (a) got rid of LogicalRepBeginPrepareData and instead used
> LogicalRepPreparedTxnData, (b) made a number of changes in comments and
> docs, (c) ran pgindent, (d) modified tests to use standard wait_for_catch
> function and removed few tests to reduce the time and to keep regression
> tests reliable.

Hi,

When reading the code, I found some comments related to the patch here.

 * XXX Now, this can even lead to a deadlock if 
the prepare
 * transaction is waiting to get it logically 
replicated for
 * distributed 2PC. Currently, we don't have an 
in-core
 * implementation of prepares for distributed 
2PC but some
 * out-of-core logical replication solution can 
have such an
 * implementation. They need to inform users to 
not have locks
 * on catalog tables in such transactions.
 */

Since we will have in-core implementation of prepares, should we update the 
comments here ?

Best regards,
houzj

RE: Add Nullif case for eval_const_expressions_mutator

[houzj.f...@fujitsu.com]

> + if (!has_nonconst_input)
> + return ece_evaluate_expr(expr);
> 
> That's not okay without a further check to see if the comparison function used
> by the node is immutable.  Compare ScalarArrayOpExpr, for instance.

Thanks for pointing it out.
Attaching new patch with this change.

Best regards,
houzj


v4-0001-add-nullif-case-for-eval_const_expressions.patch
Description: v4-0001-add-nullif-case-for-eval_const_expressions.patch

RE: fix typo in reorderbuffer.c

[houzj.f...@fujitsu.com]

> 
> What about "Combo CID(s)", for Combo command ID?  README.parallel uses
> this term for example.

Sorry for the late reply and yes, " Combo CID(s)" looks better.
Attaching patch which replaces all styles "Combocid(s)" with " Combo CID(s)".

Best regards,
houzj


v2-0001-make-the-comments-about-combo-command-id-consistent.patch
Description:  v2-0001-make-the-comments-about-combo-command-id-consistent.patch

Re: multi-install PostgresNode

[Andrew Dunstan]

On 3/23/21 7:09 PM, Andrew Dunstan wrote:
> On 3/23/21 6:36 PM, Michael Paquier wrote:
>> On Thu, Jan 28, 2021 at 10:19:57AM -0500, Andrew Dunstan wrote:
>>> +BEGIN
>>> +{
>>> +
>>> +# putting this in a BEGIN block means it's run and checked by perl -c
>>> +
>>> +
>>> +# everything other than info and get_new_node that we need to override.
>>> +# they are all instance methods, so we can use the same template for 
>>> all.
>>> +my @instance_overrides = qw(init backup start kill9 stop reload restart
>>> +  promote logrotate safe_psql psql background_psql
>>> +  interactive_psql poll_query_until command_ok
>>> +  command_fails command_like command_checks_all
>>> +  issues_sql_like run_log pg_recvlogical_upto
>>> +);
>> No actual objections here, but it would be easy to miss the addition
>> of a new routine.  Would an exclusion filter be more adapted, aka
>> override everything except get_new_node() and info()?
>
>
> Actually, following a brief offline discussion today I've thought of a
> way that doesn't require subclassing. Will post that probably tomorrow.
>


And here it is. No subclass, no eval, no magic :-) Some of my colleagues
are a lot happier ;-)

The downside is that we need to litter PostgresNode with a bunch of
lines like:

local %ENV = %ENV;
_set_install_env($self);

The upside is that there's no longer a possibility that someone will add
a new routine to PostgresNode and forget to update the subclass.

Here is my simple test program:

#!/usr/bin/perl

use lib '/home/andrew/pgl/pg_head/src/test/perl';

# PostgresNode (via TestLib) hijacks stdout, so make a dup before it
gets a chance
use vars qw($out);
BEGIN
{
    open ($out, ">&STDOUT");
}

use PostgresNode;

my $node = PostgresNode->get_new_node('v12', install_path =>
'/home/andrew/pgl/inst.12.5711');

$ENV{PG_REGRESS} = '/bin/true'; # stupid but necessary

$node->init();

$node->start();

my $version = $node->safe_psql('postgres', 'select version()');

$node->stop();

print $out "Version: $version\n";
print $out $node->info();


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

diff --git a/src/test/perl/PostgresNode.pm b/src/test/perl/PostgresNode.pm
index 97e05993be..5eabea4b2b 100644
--- a/src/test/perl/PostgresNode.pm
+++ b/src/test/perl/PostgresNode.pm
@@ -355,6 +355,7 @@ sub info
 	print $fh "Archive directory: " . $self->archive_dir . "\n";
 	print $fh "Connection string: " . $self->connstr . "\n";
 	print $fh "Log file: " . $self->logfile . "\n";
+	print $fh "Install Path: " , $self->{_install_path} . "\n" if $self->{_install_path};
 	close $fh or die;
 	return $_info;
 }
@@ -428,6 +429,9 @@ sub init
 	my $pgdata = $self->data_dir;
 	my $host   = $self->host;
 
+	local %ENV = %ENV;
+	_set_install_env($self);
+
 	$params{allows_streaming} = 0 unless defined $params{allows_streaming};
 	$params{has_archiving}= 0 unless defined $params{has_archiving};
 
@@ -555,6 +559,9 @@ sub backup
 	my $backup_path = $self->backup_dir . '/' . $backup_name;
 	my $name= $self->name;
 
+	local %ENV = %ENV;
+	_set_install_env($self);
+
 	print "# Taking pg_basebackup $backup_name from node \"$name\"\n";
 	TestLib::system_or_bail(
 		'pg_basebackup', '-D', $backup_path, '-h',
@@ -780,22 +787,22 @@ sub start
 	my $name   = $self->name;
 	my $ret;
 
+	local %ENV = %ENV;
+	_set_install_env($self);
+
 	BAIL_OUT("node \"$name\" is already running") if defined $self->{_pid};
 
 	print("### Starting node \"$name\"\n");
 
-	{
-		# Temporarily unset PGAPPNAME so that the server doesn't
-		# inherit it.  Otherwise this could affect libpqwalreceiver
-		# connections in confusing ways.
-		local %ENV = %ENV;
-		delete $ENV{PGAPPNAME};
-
-		# Note: We set the cluster_name here, not in postgresql.conf (in
-		# sub init) so that it does not get copied to standbys.
-		$ret = TestLib::system_log('pg_ctl', '-D', $self->data_dir, '-l',
+	# Temporarily unset PGAPPNAME so that the server doesn't
+	# inherit it.  Otherwise this could affect libpqwalreceiver
+	# connections in confusing ways.
+	delete $ENV{PGAPPNAME};
+
+	# Note: We set the cluster_name here, not in postgresql.conf (in
+	# sub init) so that it does not get copied to standbys.
+	$ret = TestLib::system_log('pg_ctl', '-D', $self->data_dir, '-l',
 			$self->logfile, '-o', "--cluster-name=$name", 'start');
-	}
 
 	if ($ret != 0)
 	{
@@ -826,6 +833,10 @@ sub kill9
 	my ($self) = @_;
 	my $name = $self->name;
 	return unless defined $self->{_pid};
+
+	local %ENV = %ENV;
+	_set_install_env($self);
+
 	print "### Killing node \"$name\" using signal 9\n";
 	# kill(9, ...) fails under msys Perl 5.8.8, so fall back on pg_ctl.
 	kill(9, $self->{_pid})
@@ -852,6 +863,10 @@ sub stop
 	my $port   = $self->port;
 	my $pgdata = $self->data_dir;
 	my $name   = $self->name;
+
+	local %ENV = %ENV;
+	_set_install_env($self);
+
 	$mode = 'fast' unless defined $mode;
 	return unless def

Re: Disable WAL logging to speed up data loading

[Stephen Frost]

Greetings,

* tsunakawa.ta...@fujitsu.com (tsunakawa.ta...@fujitsu.com) wrote:
> From: Stephen Frost 
> > * tsunakawa.ta...@fujitsu.com (tsunakawa.ta...@fujitsu.com) wrote:
> > As for data loading tools, surely they support loading data into UNLOGGED
> > tables and it's certainly not hard to have a script run around and flip 
> > those
> > tables to LOGGED after they're loaded, and I do actually believe some of 
> > those
> > tools support building processes of which one step could be such a command
> > (I'm fairly confident Pentaho, in particular, does as I remember building 
> > such
> > pipelines myself...).
> 
> Oh, Pentaho has such a feature, doesn't it?  But isn't it a separate step 
> from the data output step?  Here, I assume ETL tools allow users to compose a 
> data loading job from multiple steps: data input, transformation, data 
> output, etc.  I guess the user can't directly incorporate ALTER TABLE into 
> the data output step, and has to add separate custom steps for ALTER TABLE.  
> That's burdonsome and forgettable, I think.

None of the arguments presented here has done anything to change my
opinion that adding a 'none' WAL level is a bad idea.

Thanks,

Stephen


signature.asc
Description: PGP signature

Re: [HACKERS] Custom compression methods

[Dilip Kumar]

On Wed, Mar 24, 2021 at 3:40 PM Dilip Kumar  wrote:
>
> 0001 ->shows compression method for the index attribute in index describe
> 0002 -> fix the reported bug (test case included)
>
> Apart from this, I was thinking that currently, we are allowing to
> ALTER SET COMPRESSION only for the table and matview,  IMHO it makes
> sense to allow to alter the compression method for the index column as
> well?  I mean it is just a one-line change, but just wanted to know
> the opinion from others.  It is not required for the storage because
> indexes can not have a toast table but index attributes can be
> compressed so it makes sense to allow to alter the compression method.
> Thought?

I have anyway created a patch for this as well.  Including all three
patches so we don't lose track.

0001 ->shows compression method for the index attribute in index describe
0002 -> fix the reported bug (test case included)
(optional) 0003-> Alter set compression for index column

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
From 6f83c026ddfe9cda14a9c5e965841b6cd2df0a2a Mon Sep 17 00:00:00 2001
From: Dilip Kumar 
Date: Wed, 24 Mar 2021 17:08:31 +0530
Subject: [PATCH v3 3/3] ALTER SET COMPRESSION for index columns

---
 src/backend/commands/tablecmds.c|  2 +-
 src/bin/psql/tab-complete.c |  4 ++--
 src/test/regress/expected/compression.out   | 16 
 src/test/regress/expected/compression_1.out |  6 ++
 src/test/regress/sql/compression.sql|  4 
 5 files changed, 29 insertions(+), 3 deletions(-)

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 3349bcf..6d5b79d 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -4335,7 +4335,7 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
 			pass = AT_PASS_MISC;
 			break;
 		case AT_SetCompression:	/* ALTER COLUMN SET COMPRESSION */
-			ATSimplePermissions(rel, ATT_TABLE | ATT_MATVIEW);
+			ATSimplePermissions(rel, ATT_TABLE | ATT_MATVIEW | ATT_INDEX);
 			/* This command never recurses */
 			/* No command-specific prep needed */
 			pass = AT_PASS_MISC;
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index b67f4ea..305665e 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -1772,10 +1772,10 @@ psql_completion(const char *text, int start, int end)
 	}
 	/* ALTER INDEX  ALTER COLUMN  */
 	else if (Matches("ALTER", "INDEX", MatchAny, "ALTER", "COLUMN", MatchAny))
-		COMPLETE_WITH("SET STATISTICS");
+		COMPLETE_WITH("SET");
 	/* ALTER INDEX  ALTER COLUMN  SET */
 	else if (Matches("ALTER", "INDEX", MatchAny, "ALTER", "COLUMN", MatchAny, "SET"))
-		COMPLETE_WITH("STATISTICS");
+		COMPLETE_WITH("COMPRESSION", "STATISTICS");
 	/* ALTER INDEX  ALTER COLUMN  SET STATISTICS */
 	else if (Matches("ALTER", "INDEX", MatchAny, "ALTER", "COLUMN", MatchAny, "SET", "STATISTICS"))
 	{
diff --git a/src/test/regress/expected/compression.out b/src/test/regress/expected/compression.out
index 19707fb..000983f 100644
--- a/src/test/regress/expected/compression.out
+++ b/src/test/regress/expected/compression.out
@@ -326,6 +326,22 @@ generate_series(1, 50) g), VERSION());
  expr   | text | yes  | (f1 || f2) | extended | pglz| 
 unique, btree, for table "public.cmdata2"
 
+CREATE INDEX idx2 ON cmdata2(f2);
+\d+ idx2
+Index "public.idx2"
+ Column | Type | Key? | Definition | Storage  | Compression | Stats target 
++--+--++--+-+--
+ f2 | text | yes  | f2 | extended | lz4 | 
+btree, for table "public.cmdata2"
+
+ALTER INDEX idx2 ALTER COLUMN f2 SET COMPRESSION pglz;
+\d+ idx2
+Index "public.idx2"
+ Column | Type | Key? | Definition | Storage  | Compression | Stats target 
++--+--++--+-+--
+ f2 | text | yes  | f2 | extended | pglz| 
+btree, for table "public.cmdata2"
+
 -- check data is ok
 SELECT length(f1) FROM cmdata;
  length 
diff --git a/src/test/regress/expected/compression_1.out b/src/test/regress/expected/compression_1.out
index 84b933d..761fee4 100644
--- a/src/test/regress/expected/compression_1.out
+++ b/src/test/regress/expected/compression_1.out
@@ -324,6 +324,12 @@ ERROR:  relation "cmdata2" does not exist
 LINE 1: INSERT INTO cmdata2 VALUES((SELECT array_agg(md5(g::TEXT))::...
 ^
 \d+ idx1
+CREATE INDEX idx2 ON cmdata2(f2);
+ERROR:  relation "cmdata2" does not exist
+\d+ idx2
+ALTER INDEX idx2 ALTER COLUMN f2 SET COMPRESSION pglz;
+ERROR:  relation "idx2" does not exist
+\d+ idx2
 -- check data is ok
 SELECT length(f1) FROM cmdata;
  length 
diff --git a/src/test/regress/sql/compression.sql b/src/test/regress/sql/compression.sql
index 4afd5a2..6777f11 100644
--- a/src/test/regress/sql/compression.sql
+++ b/src/test/regress/sql/compression.sql
@@ -137,6 +

Re: Autovacuum worker doesn't immediately exit on postmaster death

[Stephen Frost]

Greetings,

* Michael Paquier (mich...@paquier.xyz) wrote:
> On Mon, Mar 22, 2021 at 04:07:12PM -0400, Robert Haas wrote:
> > On Mon, Mar 22, 2021 at 1:48 PM Stephen Frost  wrote:
> >> Thanks for that.  Attached is just a rebased version with a commit
> >> message added.  If there aren't any other concerns, I'll commit this in
> >> the next few days and back-patch it.  When it comes to 12 and older,
> >> does anyone want to opine about the wait event to use?  I was thinking
> >> PG_WAIT_TIMEOUT or WAIT_EVENT_PG_SLEEP ...
> > 
> > I'm not sure if we should back-patch this, but I think if you do you
> > should just add a wait event, rather than using a generic one.
> 
> I would not back-patch that either, as this is an improvement of the
> current state.  I agree that this had better introduce a new wait
> event.  Even if this stuff gets backpatched, you won't introduce an
> ABI incompatibility with a new event as long as you add the new event
> at the end of the existing enum lists, but let's keep the wait events
> ordered on HEAD.

Adding CFI's in places that really should have them is something we
certainly have back-patched in the past, and that's just 'an improvement
of the current state' too, so I don't quite follow the argument being
made here that this shouldn't be back-patched.

I don't have any problem with adding into the older releases, at the end
of the existing lists, the same wait event that exists in 13+ for this
already.

Any other thoughts on this, particularly about back-patching or not..?

Thanks,

Stephen


signature.asc
Description: PGP signature

Re: multi-install PostgresNode

[Dagfinn Ilmari Mannsåker]

Andrew Dunstan  writes:

> And here it is. No subclass, no eval, no magic :-) Some of my colleagues
> are a lot happier ;-)
>
> The downside is that we need to litter PostgresNode with a bunch of
> lines like:
>
> local %ENV = %ENV;
> _set_install_env($self);

I think it would be even neater having a method that returns the desired
environment and then have the other methods do:

  local %ENV = $self->_get_install_env();

The function could be something like this:

sub _get_install_env
{
my $self = shift;
my $inst = $self->{_install_path};
return %ENV unless $inst;

my %install_env;
if ($TestLib::windows_os)
{
# Windows picks up DLLs from the PATH rather than 
*LD_LIBRARY_PATH
# choose the right path separator
if ($Config{osname} eq 'MSWin32')
{
$install_env{PATH} = "$inst/bin;$inst/lib;$ENV{PATH}";
}
else
{
$install_env{PATH} = "$inst/bin:$inst/lib:$ENV{PATH}";
}
}
else
{
my $dylib_name =
  $Config{osname} eq 'darwin' ? "DYLD_LIBRARY_PATH" : 
"LD_LIBRARY_PATH";
$install_env{PATH} = "$inst/bin:$ENV{PATH}";
if (exists $ENV{$dylib_name})
{
$install_env{$dylib_name} = 
"$inst/lib:$ENV{$dylib_name}";
}
else
{
$install_env{$dylib_name} = "$inst/lib";
}
}

return (%ENV, %install_env);
}

- ilmari
-- 
"A disappointingly low fraction of the human race is,
 at any given time, on fire." - Stig Sandbeck Mathisen

Re: multi-install PostgresNode

[Andrew Dunstan]

On 3/24/21 7:54 AM, Dagfinn Ilmari Mannsåker wrote:
> Andrew Dunstan  writes:
>
>> And here it is. No subclass, no eval, no magic :-) Some of my colleagues
>> are a lot happier ;-)
>>
>> The downside is that we need to litter PostgresNode with a bunch of
>> lines like:
>>
>> local %ENV = %ENV;
>> _set_install_env($self);
> I think it would be even neater having a method that returns the desired
> environment and then have the other methods do:
>
>   local %ENV = $self->_get_install_env();


Yeah, that's nice. I'll do that. Thanks.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: Feature improvement: can we add queryId for pg_catalog.pg_stat_activity view?

[Bruce Momjian]

On Wed, Mar 24, 2021 at 04:51:40PM +0800, Julien Rouhaud wrote:
> > but if you do that it'd be better to avoid
> > introducing a function with one name and renaming it in your next
> > commit.
> 
> Oops, I apparently messed a fixup when working on it.  Bruce, should I take
> care of that of do you want to?  I think you have some local modifications
> already I'd rather not miss some changes.

I have no local modifications.  Please modify the patch I posted and
repost your version, thanks.

-- 
  Bruce Momjian  https://momjian.us
  EDB  https://enterprisedb.com

  If only the physical world exists, free will is an illusion.

Re: postgres_fdw: IMPORT FOREIGN SCHEMA ... LIMIT TO (partition)

[Matthias van de Meent]

On Mon, 22 Mar 2021 at 21:16, Bernd Helmle  wrote:
>
> Hi,
>
> I took a look at this patch.

Thanks!

> Patch applies on current master.
>
> Documentation and adjusted regression tests included.
> Regression tests passes without errors.
>
> The patch changes IMPORT FOREIGN SCHEMA to explicitely allow partition
> child tables in the LIMIT TO clause of the IMPORT FOREIGN SCHEMA
> command by relaxing the checks introduced with commit [1]. The reason
> behind [1] are discussed in [2].

I should've included potentially interested parties earlier, but never
too late. Stephen, Michael, Amit, would you have an opinion on lifting
this restriction for the LIMIT TO clause, seeing your involvement in
the implementation of removing partitions from IFS?

> So the original behavior this patch wants to address was done
> intentionally, so what needs to be discussed here is whether we want to
> relax that a little. One argument for the original behavior since then
> was that it is cleaner to just automatically import the parent, which
> allows access to the childs through the foreign table anways and
> exclude partition childs when querying pg_class.

Yes, but it should be noted that the main reason that was mentioned as
a reason to exclude partitions is to not cause table catalog bloat,
and I argue that this argument is not as solid in the case of the
explicitly named tables of the LIMIT TO clause. Except if SQL standard
prescribes otherwise, I think allowing partitions in LIMIT TO clauses
is an improvement overall.

> I haven't seen demand for the implemented feature here myself, but i
> could imagine use cases where just a single child or a set of child
> tables are candidates. For example, i think it's possible that users
> can query only specific childs and want them to have imported on
> another foreign server.

I myself have had this need, in that I've had to import some
partitions manually as a result of this limitation. IMPORT FORAIGN
SCHEMA really is great when it works, but limitations like these are
crippling for some more specific use cases (e.g. allowing
long-duration read-only access to one partition in the partition tree
while also allowing the partition layout of the parents to be
modified).


With regards,

Matthias.

Re: multi-install PostgresNode

[Alvaro Herrera]

On 2021-Mar-24, Dagfinn Ilmari Mannsåker wrote:

> I think it would be even neater having a method that returns the desired
> environment and then have the other methods do:
> 
>   local %ENV = $self->_get_install_env();

Hmm, is it possible to integrate PGHOST and PGPORT handling into this
too?  Seems like it is, so the name of the routine should be something
more general (and also it should not have the quick "return unless
$inst").

-- 
Álvaro Herrera39°49'30"S 73°17'W
"How amazing is that? I call it a night and come back to find that a bug has
been identified and patched while I sleep."(Robert Davidson)
   http://archives.postgresql.org/pgsql-sql/2006-03/msg00378.php

Re: [HACKERS] logical decoding of two-phase transactions

[Amit Kapila]

On Tue, Mar 23, 2021 at 3:31 PM Peter Smith  wrote:
>
> On Tue, Mar 23, 2021 at 10:44 AM Peter Smith  wrote:
> >
> > PSA patches to fix those.
> >
>
> Hi Amit.
>
> PSA a patch to allow the ALTER SUBSCRIPTION ... REFRESH PUBLICATION to
> work when two-phase tristate is PENDING.
>
> This is necessary for the pg_dump/pg_restore scenario, or for any
> other use-case where the subscription might
> start off having no tables.
>

+ subrels = GetSubscriptionRelations(MySubscription->oid);
+
+ /*
+ * If there are no tables then leave the state as PENDING, which
+ * allows ALTER SUBSCRIPTION ... REFRESH PUBLICATION to work.
+ */
+ become_two_phase_enabled = list_length(subrels) > 0;

This code is similar at both the places it is used. Isn't it better to
move this inside AllTablesyncsReady and if required then we can change
the name of the function.


-- 
With Regards,
Amit Kapila.

Re: default result formats setting

[Emre Hasegeli]

I think this is a good feature that would be useful to JDBC and more.

I don't know the surrounding code very well, but the patch looks good to me.

I agree with Tom Lane that the name of the variable is too verbose.
Maybe "auto_binary_types" is enough.  Do we gain much by prefixing
"result_format_"?  Wouldn't we use the same variable, if we support
binary inputs one day?

It is nice that the patch comes with the test module.  The name
"libpq_extended" sounds a bit vague to me.  Maybe it's a better idea
to call it "libpq_result_format" and test "format=1" in it as well.

My last nitpicking about the names is the "test-result-format"
command.  All the rest of the test modules name the commands with
underscores.  It would be nicer if this one complies.

There is one place that needs to be updated on the Makefile of the test:

> +subdir = src/test/modules/libpq_pipeline

s/pipeline/extended/

Then the test runs successfully.

Re: Nicer error when connecting to standby with hot_standby=off

[James Coleman]

On Wed, Mar 24, 2021 at 5:55 AM Fujii Masao  wrote:
>
>
>
> On 2021/03/24 16:59, Alvaro Herrera wrote:
> > On 2021-Mar-24, Fujii Masao wrote:
> >
> >> On 2021/03/24 5:59, Tom Lane wrote:
> >>> Alvaro Herrera  writes:
>  FATAL:  the database system is starting up
>  DETAIL:  WAL is being applied to recover from a system crash.
>  or
>  DETAIL:  The system is applying WAL to recover from a system crash.
>  or
>  DETAIL:  The startup process is applying WAL to recover from a system 
>  crash.
> >>>
> >>> I don't think the postmaster has enough context to know if that's
> >>> actually true.  It just launches the startup process and waits for
> >>> results.  If somebody saw this during a normal (non-crash) startup,
> >>> they'd be justifiably alarmed.
> >>
> >> Yes, so logging "the database system is starting up" seems enough to me.
> >
> > No objection.
>
> Thanks! So I changed the message reported at PM_STARTUP to that one,
> based on v8 patch that James posted upthread. I also ran pgindent for
> the patch. Attached is the updated version of the patch.
>
> Barring any objection, I will commit this.

That looks good to me. Thanks for working on this.

James Coleman

Re: pg_amcheck contrib application

[Robert Haas]

On Wed, Mar 24, 2021 at 2:13 AM Mark Dilger
 wrote:
> The visibility rules fix is different in v11, relying on a visibility check 
> which more closely follows the implementation of 
> HeapTupleSatisfiesVacuumHorizon.

Hmm. The header comment you wrote says "If a tuple might not be
visible to any running transaction, then we must not check it." But, I
don't find that statement very clear: does it mean "if there could be
even one transaction to which this tuple is not visible, we must not
check it"? Or does it mean "if the number of transactions that can see
this tuple could potentially be zero, then we must not check it"? I
don't think either of those is actually what we care about. I think
what we should be saying is "if the tuple could have been inserted by
a transaction that also added a column to the table, but which
ultimately did not commit, then the table's current TupleDesc might
differ from the one used to construct this tuple, so we must not check
it."

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Re: multi-install PostgresNode

[Andrew Dunstan]

On 3/24/21 8:29 AM, Alvaro Herrera wrote:
> On 2021-Mar-24, Dagfinn Ilmari Mannsåker wrote:
>
>> I think it would be even neater having a method that returns the desired
>> environment and then have the other methods do:
>>
>>   local %ENV = $self->_get_install_env();
> Hmm, is it possible to integrate PGHOST and PGPORT handling into this
> too?  Seems like it is, so the name of the routine should be something
> more general (and also it should not have the quick "return unless
> $inst").
>


If we're going to do that we probably shouldn't special case any
particular settings, but simply take any extra arguments as extra env
settings. And if any setting has undef (e.g. PGAPPNAME) we should unset it.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: shared memory stats: high level design decisions: consistency, dropping

[Magnus Hagander]

On Tue, Mar 23, 2021 at 4:21 AM Greg Stark  wrote:
>
> On Sun, 21 Mar 2021 at 18:16, Stephen Frost  wrote:
> >
> > Greetings,
> >
> > * Tom Lane (t...@sss.pgh.pa.us) wrote:
> > > I also believe that the snapshotting behavior has advantages in terms
> > > of being able to perform multiple successive queries and get consistent
> > > results from them.  Only the most trivial sorts of analysis don't need
> > > that.
> > >
> > > In short, what you are proposing sounds absolutely disastrous for
> > > usability of the stats views, and I for one will not sign off on it
> > > being acceptable.
> > >
> > > I do think we could relax the consistency guarantees a little bit,
> > > perhaps along the lines of only caching view rows that have already
> > > been read, rather than grabbing everything up front.  But we can't
> > > just toss the snapshot concept out the window.  It'd be like deciding
> > > that nobody needs MVCC, or even any sort of repeatable read.
> >
> > This isn't the same use-case as traditional tables or relational
> > concepts in general- there aren't any foreign keys for the fields that
> > would actually be changing across these accesses to the shared memory
> > stats- we're talking about gross stats numbers like the number of
> > inserts into a table, not an employee_id column.  In short, I don't
> > agree that this is a fair comparison.
>
> I use these stats quite a bit and do lots of slicing and dicing with
> them. I don't think it's as bad as Tom says but I also don't think we
> can be quite as loosy-goosy as I think Andres or Stephen might be
> proposing either (though I note that haven't said they don't want any
> consistency at all).
>
> The cases where the consistency really matter for me is when I'm doing
> math involving more than one statistic.
>
> Typically that's ratios. E.g. with pg_stat_*_tables I routinely divide
> seq_tup_read by seq_scan or idx_tup_* by idx_scans. I also often look
> at the ratio between n_tup_upd and n_tup_hot_upd.
>
> And no, it doesn't help that these are often large numbers after a
> long time because I'm actually working with the first derivative of
> these numbers using snapshots or a time series database. So if you
> have the seq_tup_read incremented but not seq_scan incremented you
> could get a wildly incorrect calculation of "tup read per seq scan"
> which actually matters.
>
> I don't think I've ever done math across stats for different objects.
> I mean, I've plotted them together and looked at which was higher but
> I don't think that's affected by some plots having peaks slightly out
> of sync with the other. I suppose you could look at the ratio of
> access patterns between two tables and know that they're only ever
> accessed by a single code path at the same time and therefore the
> ratios would be meaningful. But I don't think users would be surprised
> to find they're not consistent that way either.

Yeah, it's important to differentiate if things can be inconsistent
within a single object, or just between objects. And I agree that in a
lot of cases, just having per-object consistent data is probably
enough.

Normally when you graph things for example, your peaks will look
across >1 sample point anyway, and in that case it doesn't much matter
does it?

But if we said we try to offer per-object consistency only, then for
example the idx_scans value in the tables view may see changes to some
but not all indexes on that table. Would that be acceptable?

-- 
 Magnus Hagander
 Me: https://www.hagander.net/
 Work: https://www.redpill-linpro.com/

Re: PoC/WIP: Extended statistics on expressions

[Dean Rasheed]

On Wed, 24 Mar 2021 at 10:22, Tomas Vondra
 wrote:
>
> Thanks, it seems to be some thinko in handling in PlaceHolderVars, which
> seem to break the code's assumptions about varnos. This fixes it for me,
> but I need to look at it more closely.
>

I think that makes sense.

Reviewing the docs, I noticed a couple of omissions, and had a few
other suggestions (attached).

Regards,
Dean
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
new file mode 100644
index dadca67..382cbd7
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -7377,6 +7377,15 @@ SCRAM-SHA-256$

Re: shared memory stats: high level design decisions: consistency, dropping

[Magnus Hagander]

On Sun, Mar 21, 2021 at 11:34 PM Andres Freund  wrote:
>
> Hi,
>
> On 2021-03-21 12:14:35 -0400, Tom Lane wrote:
> > Andres Freund  writes:
> > > 1) What kind of consistency do we want from the pg_stats_* views?
> >
> > That's a hard choice to make.  But let me set the record straight:
> > when we did the initial implementation, the stats snapshotting behavior
> > was considered a FEATURE, not an "efficiency hack required by the old
> > storage model".
>
> Oh - sorry for misstating that then. I did try to look for the origins of the
> approach, and all that I found was that it'd be too expensive to do multiple
> stats file reads.
>
>
> > If I understand what you are proposing, all stats views would become
> > completely volatile, without even within-query consistency.  That really
> > is not gonna work.  As an example, you could get not-even-self-consistent
> > results from a join to a stats view if the planner decides to implement
> > it as a nestloop with the view on the inside.
>
> I don't really think it's a problem that's worth incuring that much cost to
> prevent. We already have that behaviour for a number of of the pg_stat_* 
> views,
> e.g. pg_stat_xact_all_tables, pg_stat_replication.

Aren't those both pretty bad examples though?

pg_stat_xact_all_tables surely is within-query consistent, and would
be pretty useless if it wwas within-transaction consistent?

pg_stat_replication is a snapshot of what things are right now (like
pg_stat_activity), and not collected statistics.

Maybe there's inconsistency in that they should've had a different
name to separate it out, but fundamentally having xact consistent
views there would be a bad thing, no?


> If the cost were low - or we can find a reasonable way to get to low costs - I
> think it'd be worth preserving for backward compatibility's sake alone.  From
> an application perspective, I actually rarely want that behaviour for stats
> views - I'm querying them to get the most recent information, not an older
> snapshot. And in the cases I do want snapshots, I'd want them for longer than 
> a
> transaction.

I agree in general, but I'd want them to be *query-consistent*, not
*transaction-consistent*. But the question is as you say, am I willing
to pay for that. Less certain of that.


> There's just a huge difference between being able to access a table's stats in
> O(1) time, or having a single stats access be O(database-objects).
>
> And that includes accesses to things like pg_stat_bgwriter, pg_stat_database
> (for IO over time stats etc) that often are monitored at a somewhat high
> frequency - they also pay the price of reading in all object stats.  On my
> example database with 1M tables it takes 0.4s to read pg_stat_database.

IMV, singeling things out into "larger groups" would be one perfectly
acceptable compromise. That is, say that pg_stat_user_tables can be
inconsistent vs with pg_stat_bgwriter, but it cannot be inconsistent
with itself.

Basically anything that's "global" seems like it could be treated that
way, independent of each other.

For relations and such having a way to get just a single relation
stats or a number of them that will be consistent with each other
without getting all of them, could also be a reasonable optimization.
Mabye an SRF that takes an array of oids as a parameter and returns
consistent data across those, without having to copy/mess with the
rest?


> We currently also fetch the full stats in places like autovacuum.c. Where we
> don't need repeated access to be consistent - we even explicitly force the
> stats to be re-read for every single table that's getting vacuumed.
>
> Even if we to just cache already accessed stats, places like do_autovacuum()
> would end up with a completely unnecessary cache of all tables, blowing up
> memory usage by a large amount on systems with lots of relations.

autovacuum is already dealing with things being pretty fuzzy though,
so it shouldn't matter much there?

But autovacuum might also deserve it's own interface to access the
data directly and doesn't have to follow the same one as the stats
views in this new scheme, perhaps?


> > I also believe that the snapshotting behavior has advantages in terms
> > of being able to perform multiple successive queries and get consistent
> > results from them.  Only the most trivial sorts of analysis don't need
> > that.
>
> In most cases you'd not do that in a transaction tho, and you'd need to create
> temporary tables with a snapshot of the stats anyway.

I'd say in most cases this analysis happens in snapshots anyway, and
those are snapshots unrelated to what we do in pg_stat. It's either
snapshotted to tables, or to storage in a completely separate
database.


> > In short, what you are proposing sounds absolutely disastrous for
> > usability of the stats views, and I for one will not sign off on it
> > being acceptable.
>
> :(
>
> That's why I thought it'd be important to bring this up to a wider
> audience. This has been discuss

psql lacking clearerr()

[Alvaro Herrera]

psql seems to never call clearerr() on its output file.  So if it gets
an error while printing a result, it'll show

could not print result table: Success

after each and every result, even though the output file isn't in error
state anymore.

It seems that the simplest fix is just to do clearerr() at the start of
printTable(), as in the attached.

I haven't been able to find a good reproducer.  Sometimes doing C-s C-c
does it, but I'm not sure it is fully reproducible.

-- 
Álvaro Herrera   Valdivia, Chile
diff --git a/src/fe_utils/print.c b/src/fe_utils/print.c
index e8772a278c..0d0b5ef782 100644
--- a/src/fe_utils/print.c
+++ b/src/fe_utils/print.c
@@ -3322,6 +3322,9 @@ printTable(const printTableContent *cont,
 	if (cont->opt->format == PRINT_NOTHING)
 		return;
 
+	/* Clear any prior error indicator */
+	clearerr(fout);
+
 	/* print_aligned_*() handle the pager themselves */
 	if (!is_pager &&
 		cont->opt->format != PRINT_ALIGNED &&

Re: [CLOBBER_CACHE]Server crashed with segfault 11 while executing clusterdb

[Tom Lane]

Amul Sul  writes:
> On Tue, Mar 23, 2021 at 8:59 PM Tom Lane  wrote:
>> On closer inspection, I believe the true culprit is c6b92041d,
>> which did this:
>> -   heap_sync(state->rs_new_rel);
>> +   smgrimmedsync(state->rs_new_rel->rd_smgr, MAIN_FORKNUM);
>> heap_sync was careful about opening rd_smgr, the new code not so much.

> So we also need to make sure of the
> RelationOpenSmgr() call before smgrimmedsync() as proposed previously.

I wonder if we should try to get rid of this sort of bug by banning
direct references to rd_smgr?  That is, write the above and all
similar code like

smgrimmedsync(RelationGetSmgr(state->rs_new_rel), MAIN_FORKNUM);

where we provide something like

static inline struct SMgrRelationData *
RelationGetSmgr(Relation rel)
{
if (unlikely(rel->rd_smgr == NULL))
RelationOpenSmgr(rel);
return rel->rd_smgr;
}

and then we could get rid of most or all other RelationOpenSmgr calls.

This might create more code bloat than it's really worth, but
it'd be a simple and mechanically-checkable scheme.

regards, tom lane

Re: pg_amcheck contrib application

[Robert Haas]

On Wed, Mar 24, 2021 at 9:12 AM Robert Haas  wrote:
> On Wed, Mar 24, 2021 at 2:13 AM Mark Dilger
>  wrote:
> > The visibility rules fix is different in v11, relying on a visibility check 
> > which more closely follows the implementation of 
> > HeapTupleSatisfiesVacuumHorizon.
>
> Hmm. The header comment you wrote says "If a tuple might not be
> visible to any running transaction, then we must not check it." But, I
> don't find that statement very clear: does it mean "if there could be
> even one transaction to which this tuple is not visible, we must not
> check it"? Or does it mean "if the number of transactions that can see
> this tuple could potentially be zero, then we must not check it"? I
> don't think either of those is actually what we care about. I think
> what we should be saying is "if the tuple could have been inserted by
> a transaction that also added a column to the table, but which
> ultimately did not commit, then the table's current TupleDesc might
> differ from the one used to construct this tuple, so we must not check
> it."

Hit send too soon. And I was wrong, too. Wahoo. Thinking about the
buildfarm failure, I realized that there's a second danger here,
unrelated to the possibility of different TupleDescs, which we talked
about before: if the tuple is dead, we can't safely follow any TOAST
pointers, because the TOAST chunks might disappear at any time. So
technically we could split the return value up into something
three-way: if the inserted is known to have committed, we can check
the tuple itself, because the TupleDesc has to be reasonable. And, if
the tuple is known not to be dead already, and known not to be in a
state where it could become dead while we're doing stuff, we can
follow the TOAST pointer. I'm not sure whether it's worth trying to be
that fancy or not.

If we were only concerned about the mismatched-TupleDesc problem, this
function could return true in a lot more cases. Once we get to the
comment that says "Okay, the inserter committed..." we could just
return true. Similarly, the HEAP_MOVED_IN and HEAP_MOVED_OFF cases
could just skip all the interior test and return true, because if the
tuple is being moved, the original inserter has to have committed.
Conversely, however, the !HeapTupleHeaderXminCommitted ->
TransactionIdIsCurrentTransactionId case probably ought to always
return false. One could argue otherwise: if we're the inserter, then
the only in-progress transaction that might have changed the TupleDesc
is us, so we could just consider this case to be a true return value
also, regardless of what's going on with xmax. In that case, we're not
asking "did the inserter definitely commit?" but "are the inserter's
possible DDL changes definitely visible to us?" which might be an OK
definition too.

However, the could-the-TOAST-data-disappear problem is another story.
I don't see how we can answer that question correctly with the logic
you've got here, because you have no XID threshold. Consider the case
where we reach this code:

+   if (!(tuphdr->t_infomask & HEAP_XMAX_COMMITTED))
+   {
+   if
(TransactionIdIsInProgress(HeapTupleHeaderGetRawXmax(tuphdr)))
+   return true;/*
HEAPTUPLE_DELETE_IN_PROGRESS */
+   else if
(!TransactionIdDidCommit(HeapTupleHeaderGetRawXmax(tuphdr)))
+
+   /*
+* Not in Progress, Not Committed, so either
Aborted or crashed
+*/
+   return true;/* HEAPTUPLE_LIVE */
+
+   /* At this point the xmax is known committed */
+   }

If we reach the case where the code comment says
HEAPTUPLE_DELETE_IN_PROGRESS, we know that the tuple isn't dead right
now, and so the TOAST tuples aren't dead either. But, by the time we
go try to look at the TOAST tuples, they might have become dead and
been pruned away, because the deleting transaction can commit at any
time, and after that pruning can happen at any time. Our only
guarantee that that won't happen is if the deleting XID is new enough
that it's invisible to some snapshot that our backend has registered.
That's approximately why HeapTupleSatisfiesVacuumHorizon needs to set
*dead_after in this case and one other, and I think you have the same
requirement.

I just noticed that this whole thing has another, related problem:
check_tuple_header_and_visibilty() and check_tuple_attribute() are
called from within check_tuple(), which is called while we hold a
buffer lock on the heap page. We should not be going and doing complex
operations that might take their own buffer locks - like TOAST index
checks - while we're holding an lwlock. That's going to have to be
changed so that the TOAST pointer checking happens after
UnlockReleaseBuffer(); in other words, we'll need to remember the
TOAST pointers to go look up and actually look them up after
UnlockReleaseBuffer(). But, when we do that, then the HEAPTUPLE_LIVE
case above has the s

Re: PoC/WIP: Extended statistics on expressions

[Tomas Vondra]

On 3/24/21 2:36 PM, Dean Rasheed wrote:
> On Wed, 24 Mar 2021 at 10:22, Tomas Vondra
>  wrote:
>>
>> Thanks, it seems to be some thinko in handling in PlaceHolderVars, which
>> seem to break the code's assumptions about varnos. This fixes it for me,
>> but I need to look at it more closely.
>>
> 
> I think that makes sense.
> 

AFAIK the primary issue here is that the two places disagree. While
estimate_num_groups does this

varnos = pull_varnos(root, (Node *) varshere);
if (bms_membership(varnos) == BMS_SINGLETON)
{ ... }

the add_unique_group_expr does this

varnos = pull_varnos(root, (Node *) groupexpr);

That is, one looks at the group expression, while the other look at vars
extracted from it by pull_var_clause(). Apparently for PlaceHolderVar
this can differ, causing the crash.

So we need to change one of those places - my fix tweaked the second
place to also look at the vars, but maybe we should change the other
place? Or maybe it's not the right fix for PlaceHolderVars ...

> Reviewing the docs, I noticed a couple of omissions, and had a few
> other suggestions (attached).
> 

Thanks! I'll include that in the next version of the patch.


regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: default result formats setting

[Robert Haas]

On Thu, Nov 5, 2020 at 3:49 PM Peter Eisentraut
 wrote:
> We could also make it a protocol message, but it would essentially
> implement the same thing, just again separately.  And then you'd have no
> support to inspect the current setting, test out different settings
> interactively, etc.  That seems pretty wasteful and complicated for no
> real gain.

But ... if it's just a GUC, it can be set by code on the server side
that the client knows nothing about, breaking the client. That seems
pretty bad to me.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Re: pgsql: Move tablespace path re-creation from the makefiles to pg_regres

[Robert Haas]

On Wed, Mar 24, 2021 at 5:56 AM Christoph Berg  wrote:
> Maybe creating the tablespace directory from within the testsuite
> would suffice?
>
> CREATE TABLE foo (t text);
> COPY foo FROM PROGRAM 'mkdir @testtablespace@';
> CREATE TABLESPACE regress_tblspace LOCATION '@testtablespace@';

Would that work on Windows?

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Re: default result formats setting

[Tom Lane]

Robert Haas  writes:
> But ... if it's just a GUC, it can be set by code on the server side
> that the client knows nothing about, breaking the client. That seems
> pretty bad to me.

It's impossible for the proposed patch to break *existing* clients,
because they all send requested format 0 or 1, and that is exactly
what they'll get back.

A client that is sending -1 and assuming that it will get back
a particular format could get broken if the GUC doesn't have the
value it thinks, true.  But I'd argue that such code is unreasonably
non-robust.  Can't we solve this by recommending that clients using
this feature always double-check which format they actually got?
ISTM that the use-cases for the feature involve checking what data
type you got anyway, so that's not an unreasonable added requirement.

regards, tom lane

Re: multi-install PostgresNode

[Andrew Dunstan]

On 3/24/21 9:23 AM, Andrew Dunstan wrote:
> On 3/24/21 8:29 AM, Alvaro Herrera wrote:
>> On 2021-Mar-24, Dagfinn Ilmari Mannsåker wrote:
>>
>>> I think it would be even neater having a method that returns the desired
>>> environment and then have the other methods do:
>>>
>>>   local %ENV = $self->_get_install_env();
>> Hmm, is it possible to integrate PGHOST and PGPORT handling into this
>> too?  Seems like it is, so the name of the routine should be something
>> more general (and also it should not have the quick "return unless
>> $inst").
>>
>
> If we're going to do that we probably shouldn't special case any
> particular settings, but simply take any extra arguments as extra env
> settings. And if any setting has undef (e.g. PGAPPNAME) we should unset it.
>
>


like this.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

diff --git a/src/test/perl/PostgresNode.pm b/src/test/perl/PostgresNode.pm
index 97e05993be..fa2ccf027f 100644
--- a/src/test/perl/PostgresNode.pm
+++ b/src/test/perl/PostgresNode.pm
@@ -355,6 +355,7 @@ sub info
 	print $fh "Archive directory: " . $self->archive_dir . "\n";
 	print $fh "Connection string: " . $self->connstr . "\n";
 	print $fh "Log file: " . $self->logfile . "\n";
+	print $fh "Install Path: " , $self->{_install_path} . "\n" if $self->{_install_path};
 	close $fh or die;
 	return $_info;
 }
@@ -428,6 +429,8 @@ sub init
 	my $pgdata = $self->data_dir;
 	my $host   = $self->host;
 
+	local %ENV = $self->_get_install_env();
+
 	$params{allows_streaming} = 0 unless defined $params{allows_streaming};
 	$params{has_archiving}= 0 unless defined $params{has_archiving};
 
@@ -555,6 +558,8 @@ sub backup
 	my $backup_path = $self->backup_dir . '/' . $backup_name;
 	my $name= $self->name;
 
+	local %ENV = $self->_get_install_env();
+
 	print "# Taking pg_basebackup $backup_name from node \"$name\"\n";
 	TestLib::system_or_bail(
 		'pg_basebackup', '-D', $backup_path, '-h',
@@ -784,18 +789,15 @@ sub start
 
 	print("### Starting node \"$name\"\n");
 
-	{
-		# Temporarily unset PGAPPNAME so that the server doesn't
-		# inherit it.  Otherwise this could affect libpqwalreceiver
-		# connections in confusing ways.
-		local %ENV = %ENV;
-		delete $ENV{PGAPPNAME};
-
-		# Note: We set the cluster_name here, not in postgresql.conf (in
-		# sub init) so that it does not get copied to standbys.
-		$ret = TestLib::system_log('pg_ctl', '-D', $self->data_dir, '-l',
+	# Temporarily unset PGAPPNAME so that the server doesn't
+	# inherit it.  Otherwise this could affect libpqwalreceiver
+	# connections in confusing ways.
+	local %ENV = $self->_get_install_env(PGAPPNAME => undef);
+
+	# Note: We set the cluster_name here, not in postgresql.conf (in
+	# sub init) so that it does not get copied to standbys.
+	$ret = TestLib::system_log('pg_ctl', '-D', $self->data_dir, '-l',
 			$self->logfile, '-o', "--cluster-name=$name", 'start');
-	}
 
 	if ($ret != 0)
 	{
@@ -826,6 +828,9 @@ sub kill9
 	my ($self) = @_;
 	my $name = $self->name;
 	return unless defined $self->{_pid};
+
+	local %ENV = $self->_get_install_env();
+
 	print "### Killing node \"$name\" using signal 9\n";
 	# kill(9, ...) fails under msys Perl 5.8.8, so fall back on pg_ctl.
 	kill(9, $self->{_pid})
@@ -852,6 +857,9 @@ sub stop
 	my $port   = $self->port;
 	my $pgdata = $self->data_dir;
 	my $name   = $self->name;
+
+	local %ENV = $self->_get_install_env();
+
 	$mode = 'fast' unless defined $mode;
 	return unless defined $self->{_pid};
 	print "### Stopping node \"$name\" using mode $mode\n";
@@ -874,6 +882,9 @@ sub reload
 	my $port   = $self->port;
 	my $pgdata = $self->data_dir;
 	my $name   = $self->name;
+
+	local %ENV = $self->_get_install_env();
+
 	print "### Reloading node \"$name\"\n";
 	TestLib::system_or_bail('pg_ctl', '-D', $pgdata, 'reload');
 	return;
@@ -895,15 +906,12 @@ sub restart
 	my $logfile = $self->logfile;
 	my $name= $self->name;
 
-	print "### Restarting node \"$name\"\n";
+	local %ENV = $self->_get_install_env(PGAPPNAME => undef);
 
-	{
-		local %ENV = %ENV;
-		delete $ENV{PGAPPNAME};
+	print "### Restarting node \"$name\"\n";
 
-		TestLib::system_or_bail('pg_ctl', '-D', $pgdata, '-l', $logfile,
+	TestLib::system_or_bail('pg_ctl', '-D', $pgdata, '-l', $logfile,
 			'restart');
-	}
 
 	$self->_update_pid(1);
 	return;
@@ -924,6 +932,9 @@ sub promote
 	my $pgdata  = $self->data_dir;
 	my $logfile = $self->logfile;
 	my $name= $self->name;
+
+	local %ENV = $self->_get_install_env();
+
 	print "### Promoting node \"$name\"\n";
 	TestLib::system_or_bail('pg_ctl', '-D', $pgdata, '-l', $logfile,
 		'promote');
@@ -945,6 +956,9 @@ sub logrotate
 	my $pgdata  = $self->data_dir;
 	my $logfile = $self->logfile;
 	my $name= $self->name;
+
+	local %ENV = $self->_get_install_env();
+
 	print "### Rotating log in node \"$name\"\n";
 	TestLib::system_or_bail('pg_ctl', '-D', $pgdata, '-l', $logfile,
 		'logrotate');
@@ -1117,6 +1131,14 @@ By default, all nodes use the same PG

Re: [HACKERS] Custom compression methods

[Robert Haas]

On Wed, Mar 24, 2021 at 7:45 AM Dilip Kumar  wrote:
> I have anyway created a patch for this as well.  Including all three
> patches so we don't lose track.
>
> 0001 ->shows compression method for the index attribute in index describe
> 0002 -> fix the reported bug (test case included)
> (optional) 0003-> Alter set compression for index column

As I understand it, the design idea here up until now has been that
the index's attcompression values are irrelevant and ignored and that
any compression which happens for index attributes is based either on
the table attribute's assigned attcompression value, or the default.
If that's the idea, then all of these patches are wrong.

Now, a possible alternative design would be that the index's
attcompression controls compression for the index same as a table's
does for the table. But in that case, it seems to me that these
patches are insufficient, because then we'd also need to, for example,
dump and restore the setting, which I don't think anything in these
patches or the existing code will do.

My vote, as of now, is for the first design, in which case you need to
forget about trying to get pg_attribute to have the right contents -
in fact, I think we should set all the values there to
InvalidCompressionMethod to make sure we're not relying on them
anywhere. And then you need to make sure that everything that tries to
compress an index value uses the setting from the table column or the
default, not the setting on the index column.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Re: Get memory contexts of an arbitrary backend process

[torikoshia]

On 2021-03-23 17:24, Kyotaro Horiguchi wrote:

Thanks for reviewing and suggestions!


At Mon, 22 Mar 2021 15:09:58 +0900, torikoshia
 wrote in

>> If MemoryContextStatsPrint(), i.e. showing 100 children at most is
>> enough, this hard limit may be acceptable.
> Can't this number be passed via shared memory?

The attached patch uses static shared memory to pass the number.


"pg_print_backend_memory_contexts"

That name looks like as if it returns the result as text when used on
command-line. We could have pg_get_backend_memory_context(bool
dump_to_log (or where to dump), int limit).  Or couldn't we name it
differently even in the ase we add a separate function?


Redefined pg_get_backend_memory_contexts() as
pg_get_backend_memory_contexts(pid, int max_children).

When pid equals 0, pg_get_backend_memory_contexts() prints local memory
contexts as original pg_get_backend_memory_contexts() does.
In this case, 'max_children' is ignored.

When 'pid' does not equal 0 and it is the PID of the client backend,
 memory contexts are logged through elog().



+/*
+ * MaxChildrenPerContext
+ *   Max number of children to print per one parent context.
+ */
+int  *MaxChildrenPerContext = NULL;

Perhaps it'd be better to have a struct even if it consists only of
one member.  (Aligned) C-int values are atomic so we can omit the
McxtPrintLock. (I don't think it's a problem even if it is modifed
while reading^^:)


Fixed them.


+ if(max_children <= 0)
+ {
+ ereport(WARNING,
+ (errmsg("%d is invalid value", 
max_children),
+  errhint("second parameter is the number 
of context and it must

be set to a value greater than or equal to 1")));

It's annoying to choose a number large enough when I want to dump
children unlimitedly.  Couldn't we use 0 to specify "unlimited"?


Modified as you suggested.

+ (errmsg("%d is invalid value", 
max_children),
+  errhint("second parameter is the number 
of context and it must

be set to a value greater than or equal to 1")));

For the main message, (I think) we usually spell the "%d is invalid
value" as "maximum number of children must be positive" or such.  For
the hint, we don't need a copy of the primary section of the
documentation here.


Modified it to "The maximum number of children must be greater than 0".



I think we should ERROR out for invalid parameters, at least for
max_children.  I'm not sure about pid since we might call it based on
pg_stat_activity..


Changed to ERROR out when the 'max_children' is less than 0.

Regarding pid, I left it untouched considering the consistency with 
other

signal sending functions such as pg_cancel_backend().

+ if(!SendProcSignal(pid, PROCSIG_PRINT_MEMORY_CONTEXT, 
InvalidBackendId))


We know the backendid of the process here.


Added it.



+ if (is_dst_stderr)
+ {
+ for (i = 0; i <= level; i++)
+ fprintf(stderr, "  ");

The fprintf path is used nowhere in the patch at all. It can be used
while attaching debugger but I'm not sure we need that code.  The
footprint of this patch is largely shrinked by removing it.


According to the past discussion[1], people wanted MemoryContextStats
as it was, so I think it's better that MemoryContextStats can be used
as before.



+ strcat(truncated_ident, delimiter);

strcpy is sufficient here.  And we don't need the delimiter to be a
variable.  (we can copy a string literal into truncate_ident, then
count the length of truncate_ident, instead of the delimiter
variable.)


True.

+ $current_logfiles = slurp_file($node->data_dir . 
'/current_logfiles');

...
+my $lfname = $current_logfiles;
+$lfname =~ s/^stderr //;
+chomp $lfname;

$node->logfile is the current log file name.

+ 'target PID is not PostgreSQL server process');

Maybe "check if PID check is working" or such?  And, we can do
something like the following to exercise in a more practical way.

 select pg_print_backend...(pid,) from pg_stat_activity where
backend_type = 'checkpointer';


It seems better.


As documented, the current implementation allows that when multiple
pg_print_backend_memory_contexts() called in succession or
simultaneously, max_children can be the one of another
pg_print_backend_memory_contexts().
I had tried to avoid this by adding some state information and using
before_shmem_exit() in case of process termination for cleaning up the
state information as in the patch I presented earlier, but since
kill()
returns success before the dumper called signal handler, it seemed
there were times when we couldn't clean up the state.
Since this happens only when multiple
pg_print_backend_memory_contexts()
are called and their specified number of children are different, and
the
effect is just the not intended number of children to print, it might
be
acceptab

Re: Feature improvement: can we add queryId for pg_catalog.pg_stat_activity view?

[Julien Rouhaud]

On Wed, Mar 24, 2021 at 08:13:40AM -0400, Bruce Momjian wrote:
> On Wed, Mar 24, 2021 at 04:51:40PM +0800, Julien Rouhaud wrote:
> > > but if you do that it'd be better to avoid
> > > introducing a function with one name and renaming it in your next
> > > commit.
> > 
> > Oops, I apparently messed a fixup when working on it.  Bruce, should I take
> > care of that of do you want to?  I think you have some local modifications
> > already I'd rather not miss some changes.
> 
> I have no local modifications.  Please modify the patch I posted and
> repost your version, thanks.

Ok!  I used the last version of the patch you sent and addressed the following
comments from earlier messages in attached v20:

- copyright year to 2021
- s/has has been compute/has been compute/
- use the name CleanQuerytext in the first commit

I didn't change the position of queryid in pg_stat_get_activity(), as the
"real" order is actually define in system_views.sql when creating
pg_stat_activity view.  Adding the new fields at the end of
pg_stat_get_activity() helps to keep the C code simpler and less bug prone, so
I think it's best to continue this way.

I also used the previous commit message if that helps.
>From 5df95cb13c3505d654bd480c8978fe6f5eba00bb Mon Sep 17 00:00:00 2001
From: Bruce Momjian 
Date: Mon, 22 Mar 2021 17:43:22 -0400
Subject: [PATCH v20 1/3] Move pg_stat_statements query jumbling to core.

A new compute_query_id GUC is also added, to control whether a query identifier
should be computed by the core.  It's thefore now possible to disable core
queryid computation and use pg_stat_statements with a different algorithm to
compute the query identifier by using third-party module.

To ensure that a single source of query identifier can be used and is well
defined, modules that calculate a query identifier should throw an error if
compute_query_id is enabled or if a query idenfitier was already calculated.
---
 .../pg_stat_statements/pg_stat_statements.c   | 805 +
 .../pg_stat_statements.conf   |   1 +
 doc/src/sgml/config.sgml  |  25 +
 doc/src/sgml/pgstatstatements.sgml|  20 +-
 src/backend/parser/analyze.c  |  14 +-
 src/backend/tcop/postgres.c   |   6 +-
 src/backend/utils/misc/Makefile   |   1 +
 src/backend/utils/misc/guc.c  |  10 +
 src/backend/utils/misc/postgresql.conf.sample |   1 +
 src/backend/utils/misc/queryjumble.c  | 834 ++
 src/include/parser/analyze.h  |   4 +-
 src/include/utils/guc.h   |   1 +
 src/include/utils/queryjumble.h   |  58 ++
 13 files changed, 995 insertions(+), 785 deletions(-)
 create mode 100644 src/backend/utils/misc/queryjumble.c
 create mode 100644 src/include/utils/queryjumble.h

diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index 62cccbfa44..bd8c96728c 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -8,24 +8,9 @@
  * a shared hashtable.  (We track only as many distinct queries as will fit
  * in the designated amount of shared memory.)
  *
- * As of Postgres 9.2, this module normalizes query entries.  Normalization
- * is a process whereby similar queries, typically differing only in their
- * constants (though the exact rules are somewhat more subtle than that) are
- * recognized as equivalent, and are tracked as a single entry.  This is
- * particularly useful for non-prepared queries.
- *
- * Normalization is implemented by fingerprinting queries, selectively
- * serializing those fields of each query tree's nodes that are judged to be
- * essential to the query.  This is referred to as a query jumble.  This is
- * distinct from a regular serialization in that various extraneous
- * information is ignored as irrelevant or not essential to the query, such
- * as the collations of Vars and, most notably, the values of constants.
- *
- * This jumble is acquired at the end of parse analysis of each query, and
- * a 64-bit hash of it is stored into the query's Query.queryId field.
- * The server then copies this value around, making it available in plan
- * tree(s) generated from the query.  The executor can then use this value
- * to blame query costs on the proper queryId.
+ * Starting in Postgres 9.2, this module normalized query entries.  As of
+ * Postgres 14, the normalization is done by the core if compute_query_id is
+ * enabled, or optionally by third-party modules.
  *
  * To facilitate presenting entries to users, we create "representative" query
  * strings in which constants are replaced with parameter symbols ($n), to
@@ -114,8 +99,6 @@ static const uint32 PGSS_PG_MAJOR_VERSION = PG_VERSION_NUM / 100;
 #define USAGE_DEALLOC_PERCENT	5	/* free this % of entries at once */
 #define IS_STICKY(c)	((c.calls[PGSS_PLAN] + c.calls[PGSS_EXEC]) == 0)
 
-#

Re: default result formats setting

[Robert Haas]

On Wed, Mar 24, 2021 at 10:58 AM Tom Lane  wrote:
> Robert Haas  writes:
> > But ... if it's just a GUC, it can be set by code on the server side
> > that the client knows nothing about, breaking the client. That seems
> > pretty bad to me.
>
> It's impossible for the proposed patch to break *existing* clients,
> because they all send requested format 0 or 1, and that is exactly
> what they'll get back.

OK.

> A client that is sending -1 and assuming that it will get back
> a particular format could get broken if the GUC doesn't have the
> value it thinks, true.  But I'd argue that such code is unreasonably
> non-robust.  Can't we solve this by recommending that clients using
> this feature always double-check which format they actually got?
> ISTM that the use-cases for the feature involve checking what data
> type you got anyway, so that's not an unreasonable added requirement.

I suppose that's a fair idea, but to me it still feels a bit like a
round peg in the square hole. Suppose for example that there's a
client application which wants to talk to a connection pooler which in
turn wants to talk to the server. Let's also suppose that connection
pooler isn't just a pass-through, but wants to redirect client
connections to various servers or even intercept queries and result
sets and make changes as the data passes by. It can do that by parsing
SQL and solving the halting problem, whereas if this were a
protocol-level option it would be completely doable. Now you could say
"well, by that argument, DateStyle ought to be a protocol-level
option, too," and that's pretty a pretty fair criticism of what I'm
saying here. On the other hand, I'm not too sure that wouldn't have
been the right call. Using SQL to tailor the wire protocol format
feels like some kind of layering inversion to me. I think we should be
working toward a state where it's more clear which things are "owned"
at the wire protocol level and which things are "owned" at the SQL
level, and this seems to be going in exactly the opposite direction,
and in fact probably taking things further in that direction than
we've ever gone before.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Re: truncating timestamps on arbitrary intervals

[Peter Eisentraut]

On 18.01.21 21:54, John Naylor wrote:
On Mon, Nov 23, 2020 at 1:44 PM John Naylor 
mailto:john.nay...@enterprisedb.com>> wrote:

 >
 > On Thu, Nov 12, 2020 at 9:56 AM Peter Eisentraut 
> wrote:

 > > - After reading the discussion a few times, I'm not so sure anymore
 > > whether making this a cousin of date_trunc is the right way to go.  As
 > > you mentioned, there are some behaviors specific to date_trunc that
 > > don't really make sense in date_trunc_interval, and maybe we'll have
 > > more of those.

For v10, I simplified the behavior by decoupling the behavior from 
date_trunc() and putting in some restrictions as discussed earlier. It's 
much simpler now. It could be argued that it goes too far in that 
direction, but it's easy to reason about and we can put back some 
features as we see fit.


Committed.

I noticed that some of the documentation disappeared between v9 and v10. 
 So I put that back and updated it appropriately.  I also added a few 
more test cases to cover some things that have been discussed during the 
course of this thread.


As a potential follow-up, should we perhaps add named arguments?  That 
might make the invocations easier to read, depending on taste.

Re: [HACKERS] Custom compression methods

[Dilip Kumar]

On Wed, Mar 24, 2021 at 8:41 PM Robert Haas  wrote:
>
> On Wed, Mar 24, 2021 at 7:45 AM Dilip Kumar  wrote:
> > I have anyway created a patch for this as well.  Including all three
> > patches so we don't lose track.
> >
> > 0001 ->shows compression method for the index attribute in index describe
> > 0002 -> fix the reported bug (test case included)
> > (optional) 0003-> Alter set compression for index column
>
> As I understand it, the design idea here up until now has been that
> the index's attcompression values are irrelevant and ignored and that
> any compression which happens for index attributes is based either on
> the table attribute's assigned attcompression value, or the default.
> If that's the idea, then all of these patches are wrong.

The current design is that whenever we create an index, the index's
attribute copies the attcompression from the table's attribute.  And,
while compressing the index tuple we will use the attcompression from
the index attribute.

> Now, a possible alternative design would be that the index's
> attcompression controls compression for the index same as a table's
> does for the table. But in that case, it seems to me that these
> patches are insufficient, because then we'd also need to, for example,
> dump and restore the setting, which I don't think anything in these
> patches or the existing code will do.

Yeah, you are right.

> My vote, as of now, is for the first design, in which case you need to
> forget about trying to get pg_attribute to have the right contents -
> in fact, I think we should set all the values there to
> InvalidCompressionMethod to make sure we're not relying on them
> anywhere. And then you need to make sure that everything that tries to
> compress an index value uses the setting from the table column or the
> default, not the setting on the index column.

Okay, that sounds like a reasonable design idea.  But the problem is
that in index_form_tuple we only have index tuple descriptor, not the
heap tuple descriptor. Maybe we will have to pass the heap tuple
descriptor as a parameter to index_form_tuple.   I will think more
about this that how can we do that.

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

Re: multi-install PostgresNode

[Alvaro Herrera]

On 2021-Mar-24, Andrew Dunstan wrote:

> 
> On 3/24/21 9:23 AM, Andrew Dunstan wrote:
> > On 3/24/21 8:29 AM, Alvaro Herrera wrote:

> > If we're going to do that we probably shouldn't special case any
> > particular settings, but simply take any extra arguments as extra env
> > settings. And if any setting has undef (e.g. PGAPPNAME) we should unset it.

> like this.

Hmm, I like that PGAPPNAME handling has resulted in an overall
simplification.  I'm not sure why you prefer to keep PGHOST and PGPORT
handled individually at each callsite however; why not do it like
_install, and add them to the environment always?  I doubt there's
anything that requires them *not* to be set; and if there is, it's easy
to make the claim that that's broken and should be fixed.

I'm just saying that cluttering _get_install_env() with those two
settings would result in less clutter overall.

-- 
Álvaro Herrera   Valdivia, Chile

Re: [HACKERS] Custom compression methods

[Tom Lane]

Andrew Dunstan  writes:
> On 3/20/21 3:03 PM, Tom Lane wrote:
>> I fixed up some issues in 0008/0009 (mostly cosmetic, except that
>> you forgot a server version check in dumpToastCompression) and
>> pushed that, so we can see if it makes crake happy.

> It's still produced a significant amount more difference between the
> dumps. For now I've increased the fuzz factor a bit like this:
> -   if (   ($oversion ne $this_branch && $difflines < 2000)
> +   if (   ($oversion ne $this_branch && $difflines < 2700)
> I'll try to come up with something better. Maybe just ignore lines like
> SET default_toast_compression = 'pglz';
> when taking the diff.

I see that some other buildfarm animals besides your own critters
are still failing the xversion tests, presumably because they lack
this hack :-(.

On reflection, though, I wonder if we've made pg_dump do the right
thing anyway.  There is a strong case to be made for the idea that
when dumping from a pre-14 server, it should emit
SET default_toast_compression = 'pglz';
rather than omitting any mention of the variable, which is what
I made it do in aa25d1089.  If we changed that, I think all these
diffs would go away.  Am I right in thinking that what's being
compared here is new pg_dump's dump from old server versus new
pg_dump's dump from new server?

The "strong case" goes like this: initdb a v14 cluster, change
default_toast_compression to lz4 in its postgresql.conf, then
try to pg_upgrade from an old server.  If the dump script doesn't
set default_toast_compression = 'pglz' then the upgrade will
do the wrong thing because all the tables will be recreated with
a different behavior than they had before.  IIUC, this wouldn't
result in broken data, but it still seems to me to be undesirable.
dump/restore ought to do its best to preserve the old DB state,
unless you explicitly tell it --no-toast-compression or the like.

regards, tom lane

Re: Support for NSS as a libpq TLS backend

[Jacob Champion]

On Wed, 2021-03-24 at 09:28 +0900, Michael Paquier wrote:
> On Wed, Mar 24, 2021 at 12:05:35AM +, Jacob Champion wrote:
> > I can work around it temporarily for the
> > tests, but this will be a problem if any libpq clients load up multiple
> > independent databases for use with separate connections. Anyone know if
> > this is a supported use case for NSS?
> 
> Are you referring to the case of threading here?  This should be a
> supported case, as threads created by an application through libpq
> could perfectly use completely different connection strings.
Right, but to clarify -- I was asking if *NSS* supports loading and
using separate certificate databases as part of its API. It seems like
the internals make it possible, but I don't see the public interfaces
to actually use those internals.

--Jacob

Re: Tying an object's ownership to datdba

[John Naylor]

On Mon, Dec 28, 2020 at 12:32 AM Noah Misch  wrote:
> [v2]

Hi Noah,

In the refactoring patch, there is a lingering comment reference to
roles_has_privs_of(). Aside from that, it looks good to me. A possible
thing to consider is an assert that is_admin is not null where we expect
that.

The database owner role patch looks good as well.

> I ended up blocking DDL that creates role memberships involving the new
role;
> see reasons in user.c comments.  Lifting those restrictions looked
feasible,
> but it was inessential to the mission, and avoiding unintended
consequences
> would have been tricky.  Views "information_schema.enabled_roles" and
> "information_schema.applicable_roles" list any implicit membership in
> pg_database_owner, but pg_catalog.pg_group and psql \dgS do not.  If this
> leads any reviewer to look closely at applicable_roles, note that its
behavior
> doesn't match its documentation
> (https://postgr.es/m/flat/20060728170615.gy20...@kenobi.snowman.net).

Is this something that needs fixing separately?

--
John Naylor
EDB: http://www.enterprisedb.com

Re: default result formats setting

[Tom Lane]

Robert Haas  writes:
> On Wed, Mar 24, 2021 at 10:58 AM Tom Lane  wrote:
>> A client that is sending -1 and assuming that it will get back
>> a particular format could get broken if the GUC doesn't have the
>> value it thinks, true.  But I'd argue that such code is unreasonably
>> non-robust.  Can't we solve this by recommending that clients using
>> this feature always double-check which format they actually got?
>> ISTM that the use-cases for the feature involve checking what data
>> type you got anyway, so that's not an unreasonable added requirement.

> I suppose that's a fair idea, but to me it still feels a bit like a
> round peg in the square hole. Suppose for example that there's a
> client application which wants to talk to a connection pooler which in
> turn wants to talk to the server. Let's also suppose that connection
> pooler isn't just a pass-through, but wants to redirect client
> connections to various servers or even intercept queries and result
> sets and make changes as the data passes by. It can do that by parsing
> SQL and solving the halting problem, whereas if this were a
> protocol-level option it would be completely doable. Now you could say
> "well, by that argument, DateStyle ought to be a protocol-level
> option, too," and that's pretty a pretty fair criticism of what I'm
> saying here. On the other hand, I'm not too sure that wouldn't have
> been the right call. Using SQL to tailor the wire protocol format
> feels like some kind of layering inversion to me.

I can't say that I'm 100% comfortable with it either, but the alternative
seems quite unpleasant, precisely because the client side might have
multiple layers involved.  If we make it a wire-protocol thing then
a whole lot of client API thrashing is going to ensue to transmit the
desired setting up and down the stack.  As an example, libpq doesn't
really give a darn which data format is returned: it is the application
using libpq that would want to be able to set this option.  If libpq
has to be involved in transmitting the option to the backend, then we
need a new libpq API call to tell it to do that.  Rinse and repeat
for anything that wraps libpq.  And, in the end, it's not real clear
which client-side layer *should* have control of this.  In some cases
you might want the decision to be taken quite high up, because which
format is really more efficient will depend on the total usage picture
for a given application, which low-level code like libpq wouldn't know.
Having a library decide that "this buck stops with me" is likely to be
the wrong thing.

I do not understand the structure of the client stack for JDBC, but
I wonder whether there won't be similar issues there.

As you say, DateStyle and the like are precedents for things that
*could* break application stacks, and in another universe maybe we'd
have managed them differently.  In the end though, they've been like
that for a long time and we've not heard many complaints about them.
So I'm inclined to think that that precedent says this is OK too.

BTW, I thought briefly about whether we could try to lock things down
a bit by marking the GUC as PGC_BACKEND, which would effectively mean
that clients would have to send it in the startup packet.  However,
that would verge on making it unusable for non-built-in datatypes,
for which you need to look up the OID first.  So I don't think that'd
be an improvement.

> I think we should be
> working toward a state where it's more clear which things are "owned"
> at the wire protocol level and which things are "owned" at the SQL
> level, and this seems to be going in exactly the opposite direction,

I don't think I buy the premise that there are exactly two levels
on the client side.

regards, tom lane

Re: Feature improvement: can we add queryId for pg_catalog.pg_stat_activity view?

[Alvaro Herrera]

On 2021-Mar-24, Julien Rouhaud wrote:

> From e08c9d5fc86ba722844d97000798de868890aba3 Mon Sep 17 00:00:00 2001
> From: Bruce Momjian 
> Date: Mon, 22 Mar 2021 17:43:23 -0400
> Subject: [PATCH v20 2/3] Expose queryid in pg_stat_activity and

>  src/backend/executor/execMain.c   |   9 ++
>  src/backend/executor/execParallel.c   |  14 ++-
>  src/backend/executor/nodeGather.c |   3 +-
>  src/backend/executor/nodeGatherMerge.c|   4 +-

Hmm...

I find it odd that there's executor code that acquires the current query
ID from pgstat, after having been put there by planner or ExecutorStart
itself.  Seems like a modularity violation.  I wonder if it would make
more sense to have the value maybe in struct EState (or perhaps there's
a better place -- but I don't think they have a way to reach the
QueryDesc anyhow), put there by ExecutorStart, so that places such as
execParallel, nodeGather etc don't have to fetch it from pgstat but from
EState.

-- 
Álvaro Herrera   Valdivia, Chile
"We're here to devour each other alive"(Hobbes)

Re: [HACKERS] Custom compression methods

[Robert Haas]

On Wed, Mar 24, 2021 at 11:41 AM Dilip Kumar  wrote:
> Okay, that sounds like a reasonable design idea.  But the problem is
> that in index_form_tuple we only have index tuple descriptor, not the
> heap tuple descriptor. Maybe we will have to pass the heap tuple
> descriptor as a parameter to index_form_tuple.   I will think more
> about this that how can we do that.

Another option might be to decide that the pg_attribute tuples for the
index columns always have to match the corresponding table columns.
So, if you alter with ALTER TABLE, it runs around and updates all of
the indexes to match. For expression index columns, we could store
InvalidCompressionMethod, causing index_form_tuple() to substitute the
run-time default. That kinda sucks, because it's a significant
impediment to ever reducing the lock level for ALTER TABLE .. ALTER
COLUMN .. SET COMPRESSION, but I'm not sure we have the luxury of
worrying about that problem right now.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Re: pg_upgrade failing for 200+ million Large Objects

[Jan Wieck]

On 3/23/21 4:55 PM, Tom Lane wrote:

Jan Wieck  writes:
Have we even reached a consensus yet on that doing it the way, my patch 
is proposing, is the right way to go? Like that emitting BLOB TOC 
entries into SECTION_DATA when in binary upgrade mode is a good thing? 
Or that bunching all the SQL statements for creating the blob, changing 
the ACL and COMMENT and SECLABEL all in one multi-statement-query is.


Now you're asking for actual review effort, which is a little hard
to come by towards the tail end of the last CF of a cycle.  I'm
interested in this topic, but I can't justify spending much time
on it right now.


Understood.

In any case I changed the options so that they behave the same way, the 
existing -o and -O (for old/new postmaster options) work. I don't think 
it would be wise to have option forwarding work differently between 
options for postmaster and options for pg_dump/pg_restore.



Regards, Jan

--
Jan Wieck
Principle Database Engineer
Amazon Web Services

Re: default result formats setting

[Robert Haas]

On Wed, Mar 24, 2021 at 12:01 PM Tom Lane  wrote:
> I don't think I buy the premise that there are exactly two levels
> on the client side.

Thanks for sharing your thoughts on this. I agree it's a complex
issue, and the idea that there are possibly more than two logical
levels is, for me, maybe your most interesting observation.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Re: pg_upgrade failing for 200+ million Large Objects

[Jan Wieck]

On 3/24/21 12:04 PM, Jan Wieck wrote:

In any case I changed the options so that they behave the same way, the
existing -o and -O (for old/new postmaster options) work. I don't think
it would be wise to have option forwarding work differently between
options for postmaster and options for pg_dump/pg_restore.


Attaching the actual diff might help.

--
Jan Wieck
Principle Database Engineer
Amazon Web Services
diff --git a/src/bin/pg_dump/parallel.c b/src/bin/pg_dump/parallel.c
index c7351a4..4a611d0 100644
--- a/src/bin/pg_dump/parallel.c
+++ b/src/bin/pg_dump/parallel.c
@@ -864,6 +864,11 @@ RunWorker(ArchiveHandle *AH, ParallelSlot *slot)
 	WaitForCommands(AH, pipefd);
 
 	/*
+	 * Close an eventually open BLOB batch transaction.
+	 */
+	CommitBlobTransaction((Archive *)AH);
+
+	/*
 	 * Disconnect from database and clean up.
 	 */
 	set_cancel_slot_archive(slot, NULL);
diff --git a/src/bin/pg_dump/pg_backup.h b/src/bin/pg_dump/pg_backup.h
index 0296b9b..cd8a590 100644
--- a/src/bin/pg_dump/pg_backup.h
+++ b/src/bin/pg_dump/pg_backup.h
@@ -203,6 +203,8 @@ typedef struct Archive
 	int			numWorkers;		/* number of parallel processes */
 	char	   *sync_snapshot_id;	/* sync snapshot id for parallel operation */
 
+	int			blobBatchSize;	/* # of blobs to restore per transaction */
+
 	/* info needed for string escaping */
 	int			encoding;		/* libpq code for client_encoding */
 	bool		std_strings;	/* standard_conforming_strings */
@@ -269,6 +271,7 @@ extern void WriteData(Archive *AH, const void *data, size_t dLen);
 extern int	StartBlob(Archive *AH, Oid oid);
 extern int	EndBlob(Archive *AH, Oid oid);
 
+extern void	CommitBlobTransaction(Archive *AH);
 extern void CloseArchive(Archive *AH);
 
 extern void SetArchiveOptions(Archive *AH, DumpOptions *dopt, RestoreOptions *ropt);
diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
index 1f82c64..8331e8a 100644
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -68,6 +68,7 @@ typedef struct _parallelReadyList
 	bool		sorted;			/* are valid entries currently sorted? */
 } ParallelReadyList;
 
+static int		blobBatchCount = 0;
 
 static ArchiveHandle *_allocAH(const char *FileSpec, const ArchiveFormat fmt,
 			   const int compression, bool dosync, ArchiveMode mode,
@@ -265,6 +266,8 @@ CloseArchive(Archive *AHX)
 	int			res = 0;
 	ArchiveHandle *AH = (ArchiveHandle *) AHX;
 
+	CommitBlobTransaction(AHX);
+
 	AH->ClosePtr(AH);
 
 	/* Close the output */
@@ -279,6 +282,23 @@ CloseArchive(Archive *AHX)
 
 /* Public */
 void
+CommitBlobTransaction(Archive *AHX)
+{
+	ArchiveHandle *AH = (ArchiveHandle *) AHX;
+
+	if (blobBatchCount > 0)
+	{
+		ahprintf(AH, "--\n");
+		ahprintf(AH, "-- End BLOB restore batch\n");
+		ahprintf(AH, "--\n");
+		ahprintf(AH, "COMMIT;\n\n");
+
+		blobBatchCount = 0;
+	}
+}
+
+/* Public */
+void
 SetArchiveOptions(Archive *AH, DumpOptions *dopt, RestoreOptions *ropt)
 {
 	/* Caller can omit dump options, in which case we synthesize them */
@@ -3531,6 +3551,57 @@ _printTocEntry(ArchiveHandle *AH, TocEntry *te, bool isData)
 {
 	RestoreOptions *ropt = AH->public.ropt;
 
+	/* We restore BLOBs in batches to reduce XID consumption */
+	if (strcmp(te->desc, "BLOB") == 0 && AH->public.blobBatchSize > 0)
+	{
+		if (blobBatchCount > 0)
+		{
+			/* We are inside a BLOB restore transaction */
+			if (blobBatchCount >= AH->public.blobBatchSize)
+			{
+/*
+ * We did reach the batch size with the previous BLOB.
+ * Commit and start a new batch.
+ */
+ahprintf(AH, "--\n");
+ahprintf(AH, "-- BLOB batch size reached\n");
+ahprintf(AH, "--\n");
+ahprintf(AH, "COMMIT;\n");
+ahprintf(AH, "BEGIN;\n\n");
+
+blobBatchCount = 1;
+			}
+			else
+			{
+/* This one still fits into the current batch */
+blobBatchCount++;
+			}
+		}
+		else
+		{
+			/* Not inside a transaction, start a new batch */
+			ahprintf(AH, "--\n");
+			ahprintf(AH, "-- Start BLOB restore batch\n");
+			ahprintf(AH, "--\n");
+			ahprintf(AH, "BEGIN;\n\n");
+
+			blobBatchCount = 1;
+		}
+	}
+	else
+	{
+		/* Not a BLOB. If we have a BLOB batch open, close it. */
+		if (blobBatchCount > 0)
+		{
+			ahprintf(AH, "--\n");
+			ahprintf(AH, "-- End BLOB restore batch\n");
+			ahprintf(AH, "--\n");
+			ahprintf(AH, "COMMIT;\n\n");
+
+			blobBatchCount = 0;
+		}
+	}
+
 	/* Select owner, schema, tablespace and default AM as necessary */
 	_becomeOwner(AH, te);
 	_selectOutputSchema(AH, te->namespace);
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index f8bec3f..f153f08 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -165,12 +165,20 @@ static void guessConstraintInheritance(TableInfo *tblinfo, int numTables);
 static void dumpComment(Archive *fout, const char *type, const char *name,
 		const char *namespace, const char *owner,
 		CatalogId catalogId, int subid, DumpId dumpId);
+static bool dumpCommentQuery(Archive *fout, PQExpBuffer 

Re: [HACKERS] Custom compression methods

[Dilip Kumar]

On Wed, Mar 24, 2021 at 9:32 PM Robert Haas  wrote:
>
> On Wed, Mar 24, 2021 at 11:41 AM Dilip Kumar  wrote:
> > Okay, that sounds like a reasonable design idea.  But the problem is
> > that in index_form_tuple we only have index tuple descriptor, not the
> > heap tuple descriptor. Maybe we will have to pass the heap tuple
> > descriptor as a parameter to index_form_tuple.   I will think more
> > about this that how can we do that.
>
> Another option might be to decide that the pg_attribute tuples for the
> index columns always have to match the corresponding table columns.
> So, if you alter with ALTER TABLE, it runs around and updates all of
> the indexes to match. For expression index columns, we could store
> InvalidCompressionMethod, causing index_form_tuple() to substitute the
> run-time default. That kinda sucks, because it's a significant
> impediment to ever reducing the lock level for ALTER TABLE .. ALTER
> COLUMN .. SET COMPRESSION, but I'm not sure we have the luxury of
> worrying about that problem right now.

Actually, we are already doing this, I mean ALTER TABLE .. ALTER
COLUMN .. SET COMPRESSION is already updating the compression method
of the index attribute.  So 0003 doesn't make sense, sorry for the
noise.  However, 0001 and 0002 are still valid, or do you think that
we don't want 0001 also?  If we don't need 0001 also then we need to
update the test output for 0002 slightly.

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

Re: [HACKERS] Custom compression methods

[Robert Haas]

On Wed, Mar 24, 2021 at 11:42 AM Tom Lane  wrote:
> On reflection, though, I wonder if we've made pg_dump do the right
> thing anyway.  There is a strong case to be made for the idea that
> when dumping from a pre-14 server, it should emit
> SET default_toast_compression = 'pglz';
> rather than omitting any mention of the variable, which is what
> I made it do in aa25d1089.  If we changed that, I think all these
> diffs would go away.  Am I right in thinking that what's being
> compared here is new pg_dump's dump from old server versus new
> pg_dump's dump from new server?
>
> The "strong case" goes like this: initdb a v14 cluster, change
> default_toast_compression to lz4 in its postgresql.conf, then
> try to pg_upgrade from an old server.  If the dump script doesn't
> set default_toast_compression = 'pglz' then the upgrade will
> do the wrong thing because all the tables will be recreated with
> a different behavior than they had before.  IIUC, this wouldn't
> result in broken data, but it still seems to me to be undesirable.
> dump/restore ought to do its best to preserve the old DB state,
> unless you explicitly tell it --no-toast-compression or the like.

This feels a bit like letting the tail wag the dog, because one might
reasonably guess that the user's intention in such a case was to
switch to using LZ4, and we've subverted that intention by deciding
that we know better. I wouldn't blame someone for thinking that using
--no-toast-compression with a pre-v14 server ought to have no effect,
but with your proposal here, it would. Furthermore, IIUC, the user has
no way of passing --no-toast-compression through to pg_upgrade, so
they're just going to have to do the upgrade and then fix everything
manually afterward to the state that they intended to have all along.
Now, on the other hand, if they wanted to make practically any other
kind of change while upgrading, they'd have to do something like that
anyway, so I guess this is no worse.

But also ... aren't we just doing this to work around a test case that
isn't especially good in the first place? Counting the number of lines
in the diff between A and B is an extremely crude proxy for "they're
similar enough that we probably haven't broken anything."

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Re: PoC/WIP: Extended statistics on expressions

[Dean Rasheed]

On Wed, 24 Mar 2021 at 14:48, Tomas Vondra
 wrote:
>
> AFAIK the primary issue here is that the two places disagree. While
> estimate_num_groups does this
>
> varnos = pull_varnos(root, (Node *) varshere);
> if (bms_membership(varnos) == BMS_SINGLETON)
> { ... }
>
> the add_unique_group_expr does this
>
> varnos = pull_varnos(root, (Node *) groupexpr);
>
> That is, one looks at the group expression, while the other look at vars
> extracted from it by pull_var_clause(). Apparently for PlaceHolderVar
> this can differ, causing the crash.
>
> So we need to change one of those places - my fix tweaked the second
> place to also look at the vars, but maybe we should change the other
> place? Or maybe it's not the right fix for PlaceHolderVars ...
>

I think that it doesn't make any difference which place is changed.

This is a case of an expression with no stats. With your change,
you'll get a single GroupExprInfo containing a list of
VariableStatData's for each of it's Var's, whereas if you changed it
the other way, you'd get a separate GroupExprInfo for each Var. But I
think they'd both end up being treated the same by
estimate_multivariate_ndistinct(), since there wouldn't be any stats
matching the expression, only the individual Var's. Maybe changing the
first place would be the more bulletproof fix though.

Regards,
Dean

Re: postgres_fdw: IMPORT FOREIGN SCHEMA ... LIMIT TO (partition)

[Bernd Helmle]

Am Mittwoch, dem 24.03.2021 um 13:23 +0100 schrieb Matthias van de
Meent:
> Yes, but it should be noted that the main reason that was mentioned
> as
> a reason to exclude partitions is to not cause table catalog bloat,
> and I argue that this argument is not as solid in the case of the
> explicitly named tables of the LIMIT TO clause. Except if SQL
> standard
> prescribes otherwise, I think allowing partitions in LIMIT TO clauses
> is an improvement overall.

Don't get me wrong, i find this useful, too. Especially because it's a
very minor change in the code. And i don't see negative aspects here
currently, either (which doesn't mean there aren't some).

> 
> I myself have had this need, in that I've had to import some
> partitions manually as a result of this limitation. IMPORT FORAIGN
> SCHEMA really is great when it works, but limitations like these are
> crippling for some more specific use cases (e.g. allowing
> long-duration read-only access to one partition in the partition tree
> while also allowing the partition layout of the parents to be
> modified).

Interesting use case. 


-- 
Thanks,
Bernd

Re: PoC/WIP: Extended statistics on expressions

[Tomas Vondra]

On 3/24/21 7:24 AM, Justin Pryzby wrote:
> Most importantly, it looks like this forgets to update catalog documentation
> for stxexprs and stxkind='e'
> 

Good catch.

> It seems like you're preferring to use pluralized "statistics" in a lot of
> places that sound wrong to me.  For example:
>> Currently the first statistics wins, which seems silly.
> I can write more separately, but I think this is resolved and clarified if you
> write "statistics object" and not just "statistics".  
> 

OK "statistics object" seems better and more consistent.

>> +   Name of schema containing table
> 
> I don't know about the nearby descriptions, but this one sounds too much like 
> a
> "schema-containing" table.  Say "Name of the schema which contains the table" 
> ?
> 

I think the current spelling is OK / consistent with the other catalogs.

>> +   Name of table
> 
> Say "name of table on which the extended statistics are defined"
> 

I've used "Name of table the statistics object is defined on".

>> +   Name of extended statistics
> 
> "Name of the extended statistic object"
> 
>> +   Owner of the extended statistics
> 
> ..object
> 

OK

>> +   Expression the extended statistics is defined on
> 
> I think it should say "the extended statistic", or "the extended statistics
> object".  Maybe "..on which the extended statistic is defined"
> 

OK

>> +   of random access to the disk.  (This expression is null if the 
>> expression
>> +   data type does not have a < operator.)
> 
> expression's data type
> 

OK

>> +   much-too-small row count estimate in the first two queries. Moreover, the
> 
> maybe say "dramatically underestimates the rowcount"
> 

I've changed this to "... results in a significant underestimate of row
count".

>> +   planner has no information about relationship between the expressions, 
>> so it
> 
> the relationship
> 

OK

>> +   assumes the two WHERE and GROUP BY
>> +   conditions are independent, and multiplies their selectivities together 
>> to
>> +   arrive at a much-too-high group count estimate in the aggregate query.
> 
> severe overestimate ?
> 

OK

>> +   This is further exacerbated by the lack of accurate statistics for the
>> +   expressions, forcing the planner to use default ndistinct estimate for 
>> the
> 
> use *a default
> 

OK

>> +   expression derived from ndistinct for the column. With such statistics, 
>> the
>> +   planner recognizes that the conditions are correlated and arrives at much
>> +   more accurate estimates.
> 
> are correlated comma
> 

OK

>> +if (type->lt_opr == InvalidOid)
> 
> These could be !OidIsValid
> 

Maybe, but it's like this already. I'll leave this alone and then
fix/backpatch separately.

>> + * expressions. It's either expensive or very easy to defeat for
>> + * determined used, and there's no risk if we allow such statistics (the
>> + * statistics is useless, but harmless).
> 
> I think it's meant to say "for a determined user" ?
> 

Right.

>> + * If there are no simply-referenced columns, give the statistics an 
>> auto
>> + * dependency on the whole table.  In most cases, this will be 
>> redundant,
>> + * but it might not be if the statistics expressions contain no Vars
>> + * (which might seem strange but possible).
>> + */
>> +if (!nattnums)
>> +{
>> +ObjectAddressSet(parentobject, RelationRelationId, relid);
>> +recordDependencyOn(&myself, &parentobject, DEPENDENCY_AUTO);
>> +}
> 
> Can this be unconditional ?
> 

What would be the benefit? This behavior copied from index_create, so
I'd prefer keeping it the same for consistency reason. Presumably it's
like that for some reason (a bit of cargo cult programming, I know).

>> + * Translate the array of indexs to regular attnums for the dependency 
>> (we
> 
> sp: indexes
> 

OK

>> + * Not found a matching expression, so 
>> we can simply skip
> 
> Found no matching expr
> 

OK

>> +/* if found a matching, */
> 
> matching ..
> 

Matching dependency.

>> +examine_attribute(Node *expr)
> 
> Maybe you should rename this to something distinct ?  So it's easy to add a
> breakpoint there, for example.
> 

What would be a better name? It's not difficult to add a breakpoint
using line number, for example.

>> +stats->anl_context = CurrentMemoryContext;  /* XXX should be using
>> +
>>  * something else? */
> 
>> +boolnulls[Natts_pg_statistic];
> ...
>> + * Construct a new pg_statistic tuple
>> + */
>> +for (i = 0; i < Natts_pg_statistic; ++i)
>> +{
>> +nulls[i] = false;
>> +}
> 
> Shouldn't you just write nulls[Natts_pg_statistic] = {false};
> or at least: memset(nulls, 0, sizeof(nulls));
> 

Maybe, but it's a copy 

Re: PoC/WIP: Extended statistics on expressions

[Justin Pryzby]

On Wed, Mar 24, 2021 at 01:24:46AM -0500, Justin Pryzby wrote:
> It seems like you're preferring to use pluralized "statistics" in a lot of
> places that sound wrong to me.  For example:
> > Currently the first statistics wins, which seems silly.
> I can write more separately, but I think this is resolved and clarified if you
> write "statistics object" and not just "statistics".  

In HEAD:catalogs.sgml, pg_statistic_ext (the table) says "object":
|Name of the statistics object
|Owner of the statistics object
|An array of attribute numbers, indicating which table columns are covered by 
this statistics object;

But pg_stats_ext (the view) doesn't say "object", which sounds wrong:
|Name of extended statistics
|Owner of the extended statistics
|Names of the columns the extended statistics is defined on

Other pre-existing issues: should be singular "statistic":
doc/src/sgml/perform.sgml: Another type of statistics stored for each 
column are most-common value
doc/src/sgml/ref/psql-ref.sgml:The status of each kind of extended 
statistics is shown in a column

Pre-existing issues: doesn't say "object" but I think it should:
src/backend/commands/statscmds.c:
errmsg("statistics creation on system columns is not supported")));
src/backend/commands/statscmds.c:
errmsg("cannot have more than %d columns in statistics",
src/backend/commands/statscmds.c:* If we got here and the OID is not 
valid, it means the statistics does
src/backend/commands/statscmds.c: * Select a nonconflicting name for a new 
statistics.
src/backend/commands/statscmds.c: * Generate "name2" for a new statistics given 
the list of column names for it
src/backend/statistics/extended_stats.c:/* compute statistics 
target for this statistics */
src/backend/statistics/extended_stats.c: * attributes the statistics is defined 
on, and then the default statistics
src/backend/statistics/mcv.c: * The input is the OID of the statistics, and 
there are no rows returned if

should say "for a statistics object" or "for statistics objects"
src/backend/statistics/extended_stats.c: * target for a statistics objects 
(from the object target, attribute targets

Your patch adds these:

Should say "object":
+* Check if we actually have a matching statistics for the expression.  

   
+   /* evaluate expressions (if the statistics has any) */  

   
+* for the extended statistics. The second option seems more 
reasonable. 
  
+* the statistics had all options enabled on the original 
version.
 
+* But if the statistics is defined on just a single column, it 
has to  
   
+   /* has the statistics expressions? */   

   
+   /* expression - see if it's in the statistics */

   
+* column(s) the statistics depends on. 
 Also require all   
   
+* statistics is defined on more than one column/expression).   

   
+* statistics is useless, but harmless).

   
+* If there are no simply-referenced columns, give the statistics an 
auto
  


+* Then the first statistics matches no expressions and 
3 vars, 
  

Re: [HACKERS] Custom compression methods

[Robert Haas]

On Mon, Mar 22, 2021 at 4:57 PM Robert Haas  wrote:
> > Fixed.
>
> Fixed some more.

Committed.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Re: [HACKERS] Custom compression methods

[Tom Lane]

Robert Haas  writes:
> On Wed, Mar 24, 2021 at 11:42 AM Tom Lane  wrote:
>> On reflection, though, I wonder if we've made pg_dump do the right
>> thing anyway.  There is a strong case to be made for the idea that
>> when dumping from a pre-14 server, it should emit
>> SET default_toast_compression = 'pglz';
>> rather than omitting any mention of the variable, which is what
>> I made it do in aa25d1089.

> But also ... aren't we just doing this to work around a test case that
> isn't especially good in the first place? Counting the number of lines
> in the diff between A and B is an extremely crude proxy for "they're
> similar enough that we probably haven't broken anything."

I wouldn't be proposing this if the xversion failures were the only
reason; making them go away is just a nice side-effect.  The core
point is that the charter of pg_dump is to reproduce the source
database's state, and as things stand we're failing to ensure we
do that.

(But yeah, we really need a better way of making this check in
the xversion tests.  I don't like the arbitrary "n lines of diff
is probably OK" business one bit.)

regards, tom lane

Re: Proposal: Save user's original authenticated identity for logging

[Jacob Champion]

On Tue, 2021-03-23 at 14:21 +0900, Michael Paquier wrote:
> I am not really sure that we need to bother about the ordering of the
> entries here, as long as we check for all of them within the same
> fragment of the log file, so I would just go down to the simplest
> solution that I posted upthread that is enough to make sure that the
> verbosity is protected.  That's what we do elsewhere, like with
> command_checks_all() and such.
With low-coverage test suites, I think it's useful to allow as little
strange behavior as possible -- in this case, printing authorization
before authentication could signal a serious bug -- but I don't feel
too strongly about it.

v10 attached, which reverts to v8 test behavior, with minor updates to
the commit message and test comment.

--Jacob
From 0b8764ac61ef37809a79ded2596c5fcd2caf25bb Mon Sep 17 00:00:00 2001
From: Jacob Champion 
Date: Mon, 8 Feb 2021 10:53:20 -0800
Subject: [PATCH v10 1/2] ssl: store client's DN in port->peer_dn

Original patch by Andrew Dunstan:

https://www.postgresql.org/message-id/fd96ae76-a8e3-ef8e-a642-a592f5b76771%40dunslane.net

but I've taken out the clientname=DN functionality; all that will be
needed for the next patch is the DN string.
---
 src/backend/libpq/be-secure-openssl.c | 53 +++
 src/include/libpq/libpq-be.h  |  1 +
 2 files changed, 47 insertions(+), 7 deletions(-)

diff --git a/src/backend/libpq/be-secure-openssl.c b/src/backend/libpq/be-secure-openssl.c
index 5ce3f27855..18321703da 100644
--- a/src/backend/libpq/be-secure-openssl.c
+++ b/src/backend/libpq/be-secure-openssl.c
@@ -551,22 +551,25 @@ aloop:
 	/* Get client certificate, if available. */
 	port->peer = SSL_get_peer_certificate(port->ssl);
 
-	/* and extract the Common Name from it. */
+	/* and extract the Common Name / Distinguished Name from it. */
 	port->peer_cn = NULL;
+	port->peer_dn = NULL;
 	port->peer_cert_valid = false;
 	if (port->peer != NULL)
 	{
 		int			len;
+		X509_NAME  *x509name = X509_get_subject_name(port->peer);
+		char	   *peer_cn;
+		char	   *peer_dn;
+		BIO		   *bio = NULL;
+		BUF_MEM*bio_buf = NULL;
 
-		len = X509_NAME_get_text_by_NID(X509_get_subject_name(port->peer),
-		NID_commonName, NULL, 0);
+		len = X509_NAME_get_text_by_NID(x509name, NID_commonName, NULL, 0);
 		if (len != -1)
 		{
-			char	   *peer_cn;
-
 			peer_cn = MemoryContextAlloc(TopMemoryContext, len + 1);
-			r = X509_NAME_get_text_by_NID(X509_get_subject_name(port->peer),
-		  NID_commonName, peer_cn, len + 1);
+			r = X509_NAME_get_text_by_NID(x509name, NID_commonName, peer_cn,
+		  len + 1);
 			peer_cn[len] = '\0';
 			if (r != len)
 			{
@@ -590,6 +593,36 @@ aloop:
 
 			port->peer_cn = peer_cn;
 		}
+
+		bio = BIO_new(BIO_s_mem());
+		if (!bio)
+		{
+			pfree(port->peer_cn);
+			port->peer_cn = NULL;
+			return -1;
+		}
+		/* use commas instead of slashes */
+		X509_NAME_print_ex(bio, x509name, 0, XN_FLAG_RFC2253);
+		BIO_get_mem_ptr(bio, &bio_buf);
+		peer_dn = MemoryContextAlloc(TopMemoryContext, bio_buf->length + 1);
+		memcpy(peer_dn, bio_buf->data, bio_buf->length);
+		peer_dn[bio_buf->length] = '\0';
+		if (bio_buf->length != strlen(peer_dn))
+		{
+			ereport(COMMERROR,
+	(errcode(ERRCODE_PROTOCOL_VIOLATION),
+	 errmsg("SSL certificate's distinguished name contains embedded null")));
+			BIO_free(bio);
+			pfree(peer_dn);
+			pfree(port->peer_cn);
+			port->peer_cn = NULL;
+			return -1;
+		}
+
+		BIO_free(bio);
+
+		port->peer_dn = peer_dn;
+
 		port->peer_cert_valid = true;
 	}
 
@@ -618,6 +651,12 @@ be_tls_close(Port *port)
 		pfree(port->peer_cn);
 		port->peer_cn = NULL;
 	}
+
+	if (port->peer_dn)
+	{
+		pfree(port->peer_dn);
+		port->peer_dn = NULL;
+	}
 }
 
 ssize_t
diff --git a/src/include/libpq/libpq-be.h b/src/include/libpq/libpq-be.h
index 30fb4e613d..d970277894 100644
--- a/src/include/libpq/libpq-be.h
+++ b/src/include/libpq/libpq-be.h
@@ -190,6 +190,7 @@ typedef struct Port
 	 */
 	bool		ssl_in_use;
 	char	   *peer_cn;
+	char	   *peer_dn;
 	bool		peer_cert_valid;
 
 	/*
-- 
2.25.1

From ac4812f4c30456849462d77982f9ce8139ba51a4 Mon Sep 17 00:00:00 2001
From: Jacob Champion 
Date: Wed, 3 Feb 2021 11:42:05 -0800
Subject: [PATCH v10 2/2] Log authenticated identity from all auth backends

The "authenticated identity" is the string used by an auth method to
identify a particular user. In many common cases this is the same as the
Postgres username, but for some third-party auth methods, the identifier
in use may be shortened or otherwise translated (e.g. through pg_ident
mappings) before the server stores it.

To help DBAs see who has actually interacted with the system, store the
original identity when authentication succeeds, and expose it via the
log_connections setting. The log entries look something like this
example (where a local user named "pchampion" is connecting to the
database as the "admin" user):

LOG:  connection received: host=[local]
LOG:  connection authenticated

Re: PoC/WIP: Extended statistics on expressions

[Tomas Vondra]

On 3/24/21 5:28 PM, Dean Rasheed wrote:
> On Wed, 24 Mar 2021 at 14:48, Tomas Vondra
>  wrote:
>>
>> AFAIK the primary issue here is that the two places disagree. While
>> estimate_num_groups does this
>>
>> varnos = pull_varnos(root, (Node *) varshere);
>> if (bms_membership(varnos) == BMS_SINGLETON)
>> { ... }
>>
>> the add_unique_group_expr does this
>>
>> varnos = pull_varnos(root, (Node *) groupexpr);
>>
>> That is, one looks at the group expression, while the other look at vars
>> extracted from it by pull_var_clause(). Apparently for PlaceHolderVar
>> this can differ, causing the crash.
>>
>> So we need to change one of those places - my fix tweaked the second
>> place to also look at the vars, but maybe we should change the other
>> place? Or maybe it's not the right fix for PlaceHolderVars ...
>>
> 
> I think that it doesn't make any difference which place is changed.
> 
> This is a case of an expression with no stats. With your change,
> you'll get a single GroupExprInfo containing a list of
> VariableStatData's for each of it's Var's, whereas if you changed it
> the other way, you'd get a separate GroupExprInfo for each Var. But I
> think they'd both end up being treated the same by
> estimate_multivariate_ndistinct(), since there wouldn't be any stats
> matching the expression, only the individual Var's. Maybe changing the
> first place would be the more bulletproof fix though.
> 

Yeah, I think that's true. I'll do a bit more research / experiments.

As for the changes proposed in the create_statistics, do we really want
to use univariate / multivariate there? Yes, the terms are correct, but
I'm not sure how many people looking at CREATE STATISTICS will
understand them.


regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: Support for NSS as a libpq TLS backend

[Stephen Frost]

Greetings Jacob,

* Jacob Champion (pchamp...@vmware.com) wrote:
> On Wed, 2021-03-24 at 09:28 +0900, Michael Paquier wrote:
> > On Wed, Mar 24, 2021 at 12:05:35AM +, Jacob Champion wrote:
> > > I can work around it temporarily for the
> > > tests, but this will be a problem if any libpq clients load up multiple
> > > independent databases for use with separate connections. Anyone know if
> > > this is a supported use case for NSS?
> > 
> > Are you referring to the case of threading here?  This should be a
> > supported case, as threads created by an application through libpq
> > could perfectly use completely different connection strings.
> Right, but to clarify -- I was asking if *NSS* supports loading and
> using separate certificate databases as part of its API. It seems like
> the internals make it possible, but I don't see the public interfaces
> to actually use those internals.

Yes, this is done using SECMOD_OpenUserDB, see:

https://developer.mozilla.org/en-US/docs/Mozilla/Projects/NSS/PKCS11_Functions#SECMOD_OpenUserDB

also there's info here:

https://groups.google.com/g/mozilla.dev.tech.crypto/c/Xz6Emfcue0E

We should document that, as mentioned in the link above, the NSS find
functions will find certs in all the opened databases.  As this would
all be under one application which is linked against libpq and passing
in different values for ssl_database for different connections, this
doesn't seem like it's really that much of an issue.

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: Change default of checkpoint_completion_target

[Stephen Frost]

Greetings,

* Bossart, Nathan (bossa...@amazon.com) wrote:
> On 3/23/21, 12:19 PM, "Stephen Frost"  wrote:
> > * Bossart, Nathan (bossa...@amazon.com) wrote:
> > > LGTM.  I just have a few small wording suggestions.
> >
> > Agreed, those looked like good suggestions and so I've incorporated
> > them.
> >
> > Updated patch attached.
> 
> Looks good!

Great, pushed!  Thanks to everyone for your thoughts, comments,
suggestions, and improvments.

Stephen


signature.asc
Description: PGP signature

Re: PoC/WIP: Extended statistics on expressions

[Dean Rasheed]

On Wed, 24 Mar 2021 at 16:48, Tomas Vondra
 wrote:
>
> As for the changes proposed in the create_statistics, do we really want
> to use univariate / multivariate there? Yes, the terms are correct, but
> I'm not sure how many people looking at CREATE STATISTICS will
> understand them.
>

Hmm, I think "univariate" and "multivariate" are pretty ubiquitous,
when used to describe statistics. You could use "single-column" and
"multi-column", but then "column" isn't really right anymore, since it
might be a column or an expression. I can't think of any other terms
that fit.

Regards,
Dean

Re: PoC/WIP: Extended statistics on expressions

[Justin Pryzby]

On Wed, Mar 24, 2021 at 05:15:46PM +, Dean Rasheed wrote:
> On Wed, 24 Mar 2021 at 16:48, Tomas Vondra
>  wrote:
> >
> > As for the changes proposed in the create_statistics, do we really want
> > to use univariate / multivariate there? Yes, the terms are correct, but
> > I'm not sure how many people looking at CREATE STATISTICS will
> > understand them.
> >
> 
> Hmm, I think "univariate" and "multivariate" are pretty ubiquitous,
> when used to describe statistics. You could use "single-column" and
> "multi-column", but then "column" isn't really right anymore, since it
> might be a column or an expression. I can't think of any other terms
> that fit.

We already use "multivariate", just not in create-statistics.sgml

doc/src/sgml/perform.sgml:multivariate statistics, 
which can capture
doc/src/sgml/perform.sgml:it's impractical to compute multivariate 
statistics automatically.
doc/src/sgml/planstats.sgml: 
doc/src/sgml/planstats.sgml:   multivariate
doc/src/sgml/planstats.sgml:multivariate statistics on the two columns:
doc/src/sgml/planstats.sgml:  
doc/src/sgml/planstats.sgml:But without multivariate statistics, the 
estimate for the number of
doc/src/sgml/planstats.sgml:This section introduces multivariate variant of 
MCV
doc/src/sgml/ref/create_statistics.sgml:  and .
doc/src/sgml/release-13.sgml:2020-01-13 [eae056c19] Apply multiple multivariate 
MCV lists when possible

So I think the answer is for create-statistics to expose that word in a
user-facing way in its reference to multivariate-statistics-examples.

-- 
Justin

Re: [HACKERS] Custom compression methods

[Robert Haas]

On Wed, Mar 24, 2021 at 12:45 PM Tom Lane  wrote:
> I wouldn't be proposing this if the xversion failures were the only
> reason; making them go away is just a nice side-effect.  The core
> point is that the charter of pg_dump is to reproduce the source
> database's state, and as things stand we're failing to ensure we
> do that.

Well, that state is just a mental construct, right? In reality, there
is no such state stored anywhere in the old database. You're choosing
to attribute to it an implicit state that matches what would need to
be configured in the newer version to get the same behavior, which is
a reasonable thing to do, but it is an interpretive choice rather than
a bare fact.

I don't care very much if you want to change this, but to me it seems
slightly worse than the status quo. It's hard to imagine that someone
is going to create a new cluster, set the default to lz4, run
pg_upgrade, and then complain that the new columns ended up with lz4
as the default. It seems much more likely that they're going to
complain if the new columns *don't* end up with lz4 as the default.
And I also can't see any other scenario where imagining that the TOAST
compression property of the old database simply does not exist, rather
than being pglz implicitly, is worse.

But I could be wrong, and even if I'm right it's not a hill upon which
I wish to die.

--
Robert Haas
EDB: http://www.enterprisedb.com

Re: [HACKERS] Custom compression methods

[Justin Pryzby]

On Wed, Mar 24, 2021 at 12:24:38PM -0400, Robert Haas wrote:
> On Wed, Mar 24, 2021 at 11:42 AM Tom Lane  wrote:
> > On reflection, though, I wonder if we've made pg_dump do the right
> > thing anyway.  There is a strong case to be made for the idea that
> > when dumping from a pre-14 server, it should emit
> > SET default_toast_compression = 'pglz';
> > rather than omitting any mention of the variable, which is what
> > I made it do in aa25d1089.  If we changed that, I think all these
> > diffs would go away.  Am I right in thinking that what's being
> > compared here is new pg_dump's dump from old server versus new
> > pg_dump's dump from new server?
> >
> > The "strong case" goes like this: initdb a v14 cluster, change
> > default_toast_compression to lz4 in its postgresql.conf, then
> > try to pg_upgrade from an old server.  If the dump script doesn't
> > set default_toast_compression = 'pglz' then the upgrade will
> > do the wrong thing because all the tables will be recreated with
> > a different behavior than they had before.  IIUC, this wouldn't
> > result in broken data, but it still seems to me to be undesirable.
> > dump/restore ought to do its best to preserve the old DB state,
> > unless you explicitly tell it --no-toast-compression or the like.
> 
> This feels a bit like letting the tail wag the dog, because one might
> reasonably guess that the user's intention in such a case was to
> switch to using LZ4, and we've subverted that intention by deciding
> that we know better. I wouldn't blame someone for thinking that using
> --no-toast-compression with a pre-v14 server ought to have no effect,
> but with your proposal here, it would. Furthermore, IIUC, the user has
> no way of passing --no-toast-compression through to pg_upgrade, so
> they're just going to have to do the upgrade and then fix everything
> manually afterward to the state that they intended to have all along.
> Now, on the other hand, if they wanted to make practically any other
> kind of change while upgrading, they'd have to do something like that
> anyway, so I guess this is no worse.

I think it's not specific to pg_upgrade, but any pg_dump |pg_restore.

The analogy with tablespaces is restoring from a cluster where the tablespace
is named "vast" to one where it's named "huge".  I do this by running
PGOPTIONS=-cdefault_tablespace=huge pg_restore --no-tablespaces

So I thinks as long as --no-toast-compression does the corresponding thing, the
"restore with alternate compression" case is handled fine.

-- 
Justin

Re: truncating timestamps on arbitrary intervals

[Erik Rijkers]

> On 2021.03.24. 16:38 Peter Eisentraut  
> wrote:

> 
> Committed.
> 

'In cases full units' seems strange.

Not a native speaker but maybe the attached changes are improvements?


Erik Rijkers--- ./doc/src/sgml/func.sgml.orig	2021-03-24 18:16:01.269515354 +0100
+++ ./doc/src/sgml/func.sgml	2021-03-24 18:18:31.695819520 +0100
@@ -9907,13 +9907,13 @@

 

-In cases full units (1 minute, 1 hour, etc.), it gives the same result as
+In case of full units (1 minute, 1 hour, etc.), it gives the same result as
 the analogous date_trunc call, but the difference is
 that date_bin can truncate to an arbitrary interval.

 

-The stride interval cannot contain units of month
+The stride interval cannot contain units of a month
 or larger.

   

Re: [HACKERS] Custom compression methods

[Robert Haas]

On Wed, Mar 24, 2021 at 12:14 PM Dilip Kumar  wrote:
> Actually, we are already doing this, I mean ALTER TABLE .. ALTER
> COLUMN .. SET COMPRESSION is already updating the compression method
> of the index attribute.  So 0003 doesn't make sense, sorry for the
> noise.  However, 0001 and 0002 are still valid, or do you think that
> we don't want 0001 also?  If we don't need 0001 also then we need to
> update the test output for 0002 slightly.

It seems to me that 0002 is still not right. We can't fix the
attcompression to whatever the default is at the time the index is
created, because the default can be changed later, and there's no way
to fix index afterward. I mean, it would be fine to do it that way if
we were going to go with the other model, where the index state is
separate from the table state, either can be changed independently,
and it all gets dumped and restored. But, as it is, I think we should
be deciding how to compress new values for an expression column based
on the default_toast_compression setting at the time of compression,
not the time of index creation.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Re: [HACKERS] Custom compression methods

[Robert Haas]

On Wed, Mar 24, 2021 at 1:24 PM Justin Pryzby  wrote:
> I think it's not specific to pg_upgrade, but any pg_dump |pg_restore.
>
> The analogy with tablespaces is restoring from a cluster where the tablespace
> is named "vast" to one where it's named "huge".  I do this by running
> PGOPTIONS=-cdefault_tablespace=huge pg_restore --no-tablespaces
>
> So I thinks as long as --no-toast-compression does the corresponding thing, 
> the
> "restore with alternate compression" case is handled fine.

I think you might be missing the point. If you're using pg_dump and
pg_restore, you can pass --no-toast-compression if you want. But if
you're using pg_upgrade, and it's internally calling pg_dump
--binary-upgrade, then you don't have control over what options get
passed. So --no-toast-compression is just fine for people who are
dumping and restoring, but it's no help at all if you want to switch
TOAST compression methods while doing a pg_upgrade. However, what does
help with that is sticking with what Tom committed before rather than
changing to what he's proposing now.

If you like his current proposal, that's fine with me, as long as
we're on the same page about what happens if we adopt it.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Re: Support for NSS as a libpq TLS backend

[Jacob Champion]

On Wed, 2021-03-24 at 13:00 -0400, Stephen Frost wrote:
> * Jacob Champion (pchamp...@vmware.com) wrote:
> > Right, but to clarify -- I was asking if *NSS* supports loading and
> > using separate certificate databases as part of its API. It seems like
> > the internals make it possible, but I don't see the public interfaces
> > to actually use those internals.
> 
> Yes, this is done using SECMOD_OpenUserDB, see:
> 
> https://developer.mozilla.org/en-US/docs/Mozilla/Projects/NSS/PKCS11_Functions#SECMOD_OpenUserDB

Ah, I had assumed that the DB-specific InitContext was using this
behind the scenes; apparently not. I will give that a try, thanks!

> also there's info here:
> 
> https://groups.google.com/g/mozilla.dev.tech.crypto/c/Xz6Emfcue0E
> 
> We should document that, as mentioned in the link above, the NSS find
> functions will find certs in all the opened databases.  As this would
> all be under one application which is linked against libpq and passing
> in different values for ssl_database for different connections, this
> doesn't seem like it's really that much of an issue.

I could see this being a problem if two client certificate nicknames
collide across multiple in-use databases, maybe?

--Jacob

Re: multi-install PostgresNode

[Andrew Dunstan]

On 3/24/21 11:41 AM, Alvaro Herrera wrote:
> On 2021-Mar-24, Andrew Dunstan wrote:
>
>> On 3/24/21 9:23 AM, Andrew Dunstan wrote:
>>> On 3/24/21 8:29 AM, Alvaro Herrera wrote:
>>> If we're going to do that we probably shouldn't special case any
>>> particular settings, but simply take any extra arguments as extra env
>>> settings. And if any setting has undef (e.g. PGAPPNAME) we should unset it.
>> like this.
> Hmm, I like that PGAPPNAME handling has resulted in an overall
> simplification.  I'm not sure why you prefer to keep PGHOST and PGPORT
> handled individually at each callsite however; why not do it like
> _install, and add them to the environment always?  I doubt there's
> anything that requires them *not* to be set; and if there is, it's easy
> to make the claim that that's broken and should be fixed.
>
> I'm just saying that cluttering _get_install_env() with those two
> settings would result in less clutter overall.
>



OK, like this?


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

diff --git a/src/test/perl/PostgresNode.pm b/src/test/perl/PostgresNode.pm
index 97e05993be..a1473d220f 100644
--- a/src/test/perl/PostgresNode.pm
+++ b/src/test/perl/PostgresNode.pm
@@ -355,6 +355,7 @@ sub info
 	print $fh "Archive directory: " . $self->archive_dir . "\n";
 	print $fh "Connection string: " . $self->connstr . "\n";
 	print $fh "Log file: " . $self->logfile . "\n";
+	print $fh "Install Path: " , $self->{_install_path} . "\n" if $self->{_install_path};
 	close $fh or die;
 	return $_info;
 }
@@ -428,6 +429,8 @@ sub init
 	my $pgdata = $self->data_dir;
 	my $host   = $self->host;
 
+	local %ENV = $self->_get_install_env();
+
 	$params{allows_streaming} = 0 unless defined $params{allows_streaming};
 	$params{has_archiving}= 0 unless defined $params{has_archiving};
 
@@ -555,6 +558,8 @@ sub backup
 	my $backup_path = $self->backup_dir . '/' . $backup_name;
 	my $name= $self->name;
 
+	local %ENV = $self->_get_install_env();
+
 	print "# Taking pg_basebackup $backup_name from node \"$name\"\n";
 	TestLib::system_or_bail(
 		'pg_basebackup', '-D', $backup_path, '-h',
@@ -784,18 +789,15 @@ sub start
 
 	print("### Starting node \"$name\"\n");
 
-	{
-		# Temporarily unset PGAPPNAME so that the server doesn't
-		# inherit it.  Otherwise this could affect libpqwalreceiver
-		# connections in confusing ways.
-		local %ENV = %ENV;
-		delete $ENV{PGAPPNAME};
-
-		# Note: We set the cluster_name here, not in postgresql.conf (in
-		# sub init) so that it does not get copied to standbys.
-		$ret = TestLib::system_log('pg_ctl', '-D', $self->data_dir, '-l',
+	# Temporarily unset PGAPPNAME so that the server doesn't
+	# inherit it.  Otherwise this could affect libpqwalreceiver
+	# connections in confusing ways.
+	local %ENV = $self->_get_install_env(PGAPPNAME => undef);
+
+	# Note: We set the cluster_name here, not in postgresql.conf (in
+	# sub init) so that it does not get copied to standbys.
+	$ret = TestLib::system_log('pg_ctl', '-D', $self->data_dir, '-l',
 			$self->logfile, '-o', "--cluster-name=$name", 'start');
-	}
 
 	if ($ret != 0)
 	{
@@ -826,6 +828,9 @@ sub kill9
 	my ($self) = @_;
 	my $name = $self->name;
 	return unless defined $self->{_pid};
+
+	local %ENV = $self->_get_install_env();
+
 	print "### Killing node \"$name\" using signal 9\n";
 	# kill(9, ...) fails under msys Perl 5.8.8, so fall back on pg_ctl.
 	kill(9, $self->{_pid})
@@ -852,6 +857,9 @@ sub stop
 	my $port   = $self->port;
 	my $pgdata = $self->data_dir;
 	my $name   = $self->name;
+
+	local %ENV = $self->_get_install_env();
+
 	$mode = 'fast' unless defined $mode;
 	return unless defined $self->{_pid};
 	print "### Stopping node \"$name\" using mode $mode\n";
@@ -874,6 +882,9 @@ sub reload
 	my $port   = $self->port;
 	my $pgdata = $self->data_dir;
 	my $name   = $self->name;
+
+	local %ENV = $self->_get_install_env();
+
 	print "### Reloading node \"$name\"\n";
 	TestLib::system_or_bail('pg_ctl', '-D', $pgdata, 'reload');
 	return;
@@ -895,15 +906,12 @@ sub restart
 	my $logfile = $self->logfile;
 	my $name= $self->name;
 
-	print "### Restarting node \"$name\"\n";
+	local %ENV = $self->_get_install_env(PGAPPNAME => undef);
 
-	{
-		local %ENV = %ENV;
-		delete $ENV{PGAPPNAME};
+	print "### Restarting node \"$name\"\n";
 
-		TestLib::system_or_bail('pg_ctl', '-D', $pgdata, '-l', $logfile,
+	TestLib::system_or_bail('pg_ctl', '-D', $pgdata, '-l', $logfile,
 			'restart');
-	}
 
 	$self->_update_pid(1);
 	return;
@@ -924,6 +932,9 @@ sub promote
 	my $pgdata  = $self->data_dir;
 	my $logfile = $self->logfile;
 	my $name= $self->name;
+
+	local %ENV = $self->_get_install_env();
+
 	print "### Promoting node \"$name\"\n";
 	TestLib::system_or_bail('pg_ctl', '-D', $pgdata, '-l', $logfile,
 		'promote');
@@ -945,6 +956,9 @@ sub logrotate
 	my $pgdata  = $self->data_dir;
 	my $logfile = $self->logfile;
 	my $name= $self->name;
+
+	local %ENV = $self->_get_install_env();
+
 	print "### Rotating 

Re: [HACKERS] Custom compression methods

[Justin Pryzby]

On Wed, Mar 24, 2021 at 01:30:26PM -0400, Robert Haas wrote:
> On Wed, Mar 24, 2021 at 1:24 PM Justin Pryzby  wrote:
> > I think it's not specific to pg_upgrade, but any pg_dump |pg_restore.
> >
> > The analogy with tablespaces is restoring from a cluster where the 
> > tablespace
> > is named "vast" to one where it's named "huge".  I do this by running
> > PGOPTIONS=-cdefault_tablespace=huge pg_restore --no-tablespaces
> >
> > So I thinks as long as --no-toast-compression does the corresponding thing, 
> > the
> > "restore with alternate compression" case is handled fine.
> 
> I think you might be missing the point. If you're using pg_dump and
> pg_restore, you can pass --no-toast-compression if you want. But if

Actually, I forgot that pg_restore doesn't (can't) have --no-toast-compression.
So my analogy is broken.

> you're using pg_upgrade, and it's internally calling pg_dump
> --binary-upgrade, then you don't have control over what options get
> passed. So --no-toast-compression is just fine for people who are
> dumping and restoring, but it's no help at all if you want to switch
> TOAST compression methods while doing a pg_upgrade. However, what does
> help with that is sticking with what Tom committed before rather than
> changing to what he's proposing now.

I don't know what/any other cases support using pg_upgrade to change stuff like
the example (changing to lz4).  The way to do it is to make the changes either
before or after.  It seems weird to think that pg_upgrade would handle that.

I'm going to risk making the analogy that it's not supported to pass
--no-tablespaces from pg_upgrade to pg_dump/restore.  It certainly can't work
for --link (except in the weird case that the dirs are on the same filesystem). 
 

-- 
Justin

Re: truncating timestamps on arbitrary intervals

[John Naylor]

On Wed, Mar 24, 2021 at 11:38 AM Peter Eisentraut <
peter.eisentr...@enterprisedb.com> wrote:

> Committed.
>
> I noticed that some of the documentation disappeared between v9 and v10.
>   So I put that back and updated it appropriately.  I also added a few
> more test cases to cover some things that have been discussed during the
> course of this thread.

Thanks! I put off updating the documentation in case the latest approach
was not feature-rich enough.

> As a potential follow-up, should we perhaps add named arguments?  That
> might make the invocations easier to read, depending on taste.

I think it's quite possible some users will prefer that. All we need is to
add something like

proargnames => '{bin_width,input,origin}'

to the catalog, right?

Also, I noticed that I put in double semicolons in the new functions
somehow. I'll fix that as well.

--
John Naylor
EDB: http://www.enterprisedb.com

Re: truncating timestamps on arbitrary intervals

[John Naylor]

On Wed, Mar 24, 2021 at 1:25 PM Erik Rijkers  wrote:
>
> 'In cases full units' seems strange.
>
> Not a native speaker but maybe the attached changes are improvements?

-In cases full units (1 minute, 1 hour, etc.), it gives the same result
as
+In case of full units (1 minute, 1 hour, etc.), it gives the same
result as
 the analogous date_trunc call, but the difference
is
 that date_bin can truncate to an arbitrary
interval.


I would say "In the case of"


-The stride interval cannot contain units of
month
+The stride interval cannot contain units of a
month
 or larger.

The original seems fine to me here.

--
John Naylor
EDB: http://www.enterprisedb.com

Re: Support for NSS as a libpq TLS backend

[Stephen Frost]

Greetings,

* Jacob Champion (pchamp...@vmware.com) wrote:
> On Wed, 2021-03-24 at 13:00 -0400, Stephen Frost wrote:
> > * Jacob Champion (pchamp...@vmware.com) wrote:
> > > Right, but to clarify -- I was asking if *NSS* supports loading and
> > > using separate certificate databases as part of its API. It seems like
> > > the internals make it possible, but I don't see the public interfaces
> > > to actually use those internals.
> > 
> > Yes, this is done using SECMOD_OpenUserDB, see:
> > 
> > https://developer.mozilla.org/en-US/docs/Mozilla/Projects/NSS/PKCS11_Functions#SECMOD_OpenUserDB
> 
> Ah, I had assumed that the DB-specific InitContext was using this
> behind the scenes; apparently not. I will give that a try, thanks!
> 
> > also there's info here:
> > 
> > https://groups.google.com/g/mozilla.dev.tech.crypto/c/Xz6Emfcue0E
> > 
> > We should document that, as mentioned in the link above, the NSS find
> > functions will find certs in all the opened databases.  As this would
> > all be under one application which is linked against libpq and passing
> > in different values for ssl_database for different connections, this
> > doesn't seem like it's really that much of an issue.
> 
> I could see this being a problem if two client certificate nicknames
> collide across multiple in-use databases, maybe?

Right, in such a case either cert might get returned and it's possible
that the "wrong" one is returned and therefore the connection would end
up failing, assuming that they aren't actually the same and just happen
to be in both.

Seems like we could use SECMOD_OpenUserDB() and then pass the result
from that into PK11_ListCertsInSlot() and scan through the certs in just
the specified database to find the one we're looking for if we really
feel compelled to try and address this risk.  I've reached out to the
NSS folks to see if they have any thoughts about the best way to address
this.

Thanks,

Stephen


signature.asc
Description: PGP signature

Re: [HACKERS] Custom compression methods

[Tom Lane]

Justin Pryzby  writes:
> On Wed, Mar 24, 2021 at 01:30:26PM -0400, Robert Haas wrote:
>> ... So --no-toast-compression is just fine for people who are
>> dumping and restoring, but it's no help at all if you want to switch
>> TOAST compression methods while doing a pg_upgrade. However, what does
>> help with that is sticking with what Tom committed before rather than
>> changing to what he's proposing now.

> I don't know what/any other cases support using pg_upgrade to change stuff 
> like
> the example (changing to lz4).  The way to do it is to make the changes either
> before or after.  It seems weird to think that pg_upgrade would handle that.

Yeah; I think the charter of pg_upgrade is to reproduce the old database
state.  If you try to twiddle the process to incorporate some changes
in that state, maybe it will work, but if it breaks you get to keep both
pieces.  I surely don't wish to consider such shenanigans as supported.

But let's ignore the case of pg_upgrade and just consider a dump/restore.
I'd still say that unless you give --no-toast-compression then I would
expect the dump/restore to preserve the tables' old compression behavior.
Robert's argument that the pre-v14 database had no particular compression
behavior seems nonsensical to me.  We know exactly which compression
behavior it has.

regards, tom lane

Re: multi-install PostgresNode

[Alvaro Herrera]

On 2021-Mar-24, Andrew Dunstan wrote:

> OK, like this?

Yeah, looks good!

> +# Private routine to return a copy of the environment with the PATH and 
> (DY)LD_LIBRARY_PATH
> +# correctly set when there is an install path set for the node.
> +# Routines that call Postgres binaries need to call this routine like this:
> +#
> +#local %ENV = $self->_get_install_env{[%extra_settings]);
> +#
> +# A copy of the environmnt is taken and node's host and port settings are 
> added
> +# as PGHOST and PGPORT, Then the extra settings (if any) are applied. Any 
> setting
> +# in %extra_settings with a value that is undefined is deleted; the 
> remainder are
> +# set. Then the PATH and (DY)LD_LIBRARY_PATH are adjusted if the node's 
> install path
> +# is set, and the copy environment is returned.

There's a typo "environmnt" here, and a couple of lines appear
overlength.

> +sub _get_install_env

I'd use a name that doesn't have "install" in it -- maybe _get_env or
_get_postgres_env or _get_PostgresNode_env -- but don't really care too
much about it.


> +# The install path set in get_new_node needs to be a directory containing
> +# bin and lib subdirectories as in a standard PostgreSQL installation, so 
> this
> +# can't be used with installations where the bin and lib directories don't 
> have
> +# a common parent directory.

I've never heard of an installation where that wasn't true.  If there
was a need for that, seems like it'd be possible to set them with
{ bindir => ..., libdir => ...} but I doubt it'll ever be necessary.

-- 
Álvaro Herrera   Valdivia, Chile
"We're here to devour each other alive"(Hobbes)

Re: pl/pgsql feature request: shorthand for argument and local variable references

[Pavel Stehule]

st 24. 3. 2021 v 8:42 odesílatel Erik Rijkers  napsal:

> > On 2021.03.24. 08:09 Pavel Stehule  wrote:
> > [...]
> > [plpgsql-routine-label-20210324.patch]
>
>
> Hi,
>
> In that sgml
>
> "the functions' arguments"
>
> should be
>
> "the function's arguments"
>

fixed,

thank you for check



>
> Erik
>
diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml
index 52f60c827c..3643521091 100644
--- a/doc/src/sgml/plpgsql.sgml
+++ b/doc/src/sgml/plpgsql.sgml
@@ -292,7 +292,9 @@ $$ LANGUAGE plpgsql;
   special variables such as FOUND (see
   ).  The outer block is
   labeled with the function's name, meaning that parameters and special
-  variables can be qualified with the function's name.
+  variables can be qualified with the function's name. The name of this outer
+  block can be changed by inserting special command 
+  #routine_label new_name at the start of the function.
  
 
 
@@ -435,6 +437,31 @@ $$ LANGUAGE plpgsql;
  
 
 
+
+ The function's argument can be qualified with the function name:
+
+CREATE FUNCTION sales_tax(subtotal real) RETURNS real AS $$
+BEGIN
+RETURN sales_tax.subtotal * 0.06;
+END;
+$$ LANGUAGE plpgsql;
+
+
+
+
+ Sometimes the function name is too long and it can be more practical to use
+ some shorter label. The top namespace label can be changed (along with
+ the function's arguments) using the option routine_label:
+
+CREATE FUNCTION sales_tax(subtotal real) RETURNS real AS $$
+#routine_label s
+BEGIN
+RETURN s.subtotal * 0.06;
+END;
+$$ LANGUAGE plpgsql;
+
+
+
  
   Some more examples:
 
diff --git a/src/pl/plpgsql/src/pl_comp.c b/src/pl/plpgsql/src/pl_comp.c
index ce8d97447d..d32e050c32 100644
--- a/src/pl/plpgsql/src/pl_comp.c
+++ b/src/pl/plpgsql/src/pl_comp.c
@@ -378,6 +378,10 @@ do_compile(FunctionCallInfo fcinfo,
 	 */
 	plpgsql_ns_init();
 	plpgsql_ns_push(NameStr(procStruct->proname), PLPGSQL_LABEL_BLOCK);
+
+	/* save top ns for possibility to alter top label */
+	function->root_ns = plpgsql_ns_top();
+
 	plpgsql_DumpExecTree = false;
 	plpgsql_start_datums();
 
diff --git a/src/pl/plpgsql/src/pl_funcs.c b/src/pl/plpgsql/src/pl_funcs.c
index 919b968826..7132da35d1 100644
--- a/src/pl/plpgsql/src/pl_funcs.c
+++ b/src/pl/plpgsql/src/pl_funcs.c
@@ -101,6 +101,7 @@ plpgsql_ns_additem(PLpgSQL_nsitem_type itemtype, int itemno, const char *name)
 	nse->itemtype = itemtype;
 	nse->itemno = itemno;
 	nse->prev = ns_top;
+	nse->alias = NULL;
 	strcpy(nse->name, name);
 	ns_top = nse;
 }
@@ -141,7 +142,7 @@ plpgsql_ns_lookup(PLpgSQL_nsitem *ns_cur, bool localmode,
 			 nsitem->itemtype != PLPGSQL_NSTYPE_LABEL;
 			 nsitem = nsitem->prev)
 		{
-			if (strcmp(nsitem->name, name1) == 0)
+			if (strcmp(nsitem->alias ? nsitem->alias : nsitem->name, name1) == 0)
 			{
 if (name2 == NULL ||
 	nsitem->itemtype != PLPGSQL_NSTYPE_VAR)
@@ -155,13 +156,13 @@ plpgsql_ns_lookup(PLpgSQL_nsitem *ns_cur, bool localmode,
 
 		/* Check this level for qualified match to variable name */
 		if (name2 != NULL &&
-			strcmp(nsitem->name, name1) == 0)
+			strcmp(nsitem->alias ? nsitem->alias : nsitem->name, name1) == 0)
 		{
 			for (nsitem = ns_cur;
  nsitem->itemtype != PLPGSQL_NSTYPE_LABEL;
  nsitem = nsitem->prev)
 			{
-if (strcmp(nsitem->name, name2) == 0)
+if (strcmp(nsitem->alias ? nsitem->alias : nsitem->name, name2) == 0)
 {
 	if (name3 == NULL ||
 		nsitem->itemtype != PLPGSQL_NSTYPE_VAR)
@@ -197,7 +198,7 @@ plpgsql_ns_lookup_label(PLpgSQL_nsitem *ns_cur, const char *name)
 	while (ns_cur != NULL)
 	{
 		if (ns_cur->itemtype == PLPGSQL_NSTYPE_LABEL &&
-			strcmp(ns_cur->name, name) == 0)
+			strcmp(ns_cur->alias ? ns_cur->alias : ns_cur->name, name) == 0)
 			return ns_cur;
 		ns_cur = ns_cur->prev;
 	}
diff --git a/src/pl/plpgsql/src/pl_gram.y b/src/pl/plpgsql/src/pl_gram.y
index 34e0520719..f3536d75ae 100644
--- a/src/pl/plpgsql/src/pl_gram.y
+++ b/src/pl/plpgsql/src/pl_gram.y
@@ -333,6 +333,7 @@ static	void			check_raise_parameters(PLpgSQL_stmt_raise *stmt);
 %token 	K_RETURNED_SQLSTATE
 %token 	K_REVERSE
 %token 	K_ROLLBACK
+%token 	K_ROUTINE_LABEL
 %token 	K_ROW_COUNT
 %token 	K_ROWTYPE
 %token 	K_SCHEMA
@@ -393,6 +394,24 @@ comp_option		: '#' K_OPTION K_DUMP
 	{
 		plpgsql_curr_compile->resolve_option = PLPGSQL_RESOLVE_COLUMN;
 	}
+| '#' K_ROUTINE_LABEL any_identifier
+	{
+		if (!plpgsql_curr_compile->root_ns)
+			ereport(ERROR,
+	(errcode(ERRCODE_SYNTAX_ERROR),
+	 errmsg("The command \"routine_label\" cannot be used in inline block"),
+	 parser_errposition(@1)));
+
+		/* Don't allow more top labels in rout

Re: TRUNCATE on foreign table

[Fujii Masao]

On 2021/03/13 18:57, Kazutaka Onishi wrote:

I have fixed the patch to pass check-world test. :D


Thanks for updating the patch! Here are some review comments from me.


  By default all foreign tables using postgres_fdw are 
assumed
  to be updatable.  This may be overridden using the following option:

In postgres-fdw.sgml, "and truncatable" should be appended into
the above first description? Also "option" in the second description
should be a plural form "options"?


 TRUNCATE is not currently supported for foreign tables.
 This implies that if a specified table has any descendant tables that are
 foreign, the command will fail.

truncate.sgml should be updated because, for example, it contains
the above descriptions.


+ frels_extra is same length with
+ frels_list, that delivers extra information of
+ the context where the foreign-tables are truncated.
+

Don't we need to document the detail information about frels_extra?
Otherwise the developers of FDW would fail to understand how to
handle the frels_extra when trying to make their FDWs support TRUNCATE.


+   relids_extra = lappend_int(relids_extra, (recurse ? 0 : 1));
+   relids_extra = lappend_int(relids_extra, -1);

postgres_fdw determines whether to specify ONLY or not by checking
whether the passed extra value is zero or not. That is, for example,
using only 0 and 1 for extra values is enough for the purpose. But
ExecuteTruncate() sets three values 0, -1 and 1 as extra ones. Why are
these three values necessary?


With the patch, if both local and foreign tables are specified as
the target tables to truncate, TRUNCATE command tries to truncate
foreign tables after truncating local ones. That is, if "truncatable"
option is set to false or enough permission to truncate is not granted
yet in the foreign server, an error will be thrown after the local tables
are truncated. I don't think this is good order of processings. IMO,
instead, we should check whether foreign tables can be truncated
before any actual truncation operations. For example, we can easily
do that by truncate foreign tables before local ones. Thought?


XLOG_HEAP_TRUNCATE record is written even for the truncation of
a foreign table. Why is this necessary?

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

Re: making update/delete of inheritance trees scale better

[Tom Lane]

I wrote:
> Yeah, it's on my to-do list for this CF, but I expect it's going to
> take some concentrated study and other things keep intruding :-(.

I've started to look at this seriously, and I wanted to make a note
about something that I think we should try to do, but there seems
little hope of shoehorning it in for v14.

That "something" is that ideally, the ModifyTable node should pass
only the updated column values to the table AM or FDW, and let that
lower-level code worry about reconstructing a full tuple by
re-fetching the unmodified columns.  When I first proposed this
concept, I'd imagined that maybe we could avoid the additional tuple
read that this implementation requires by combining it with the
tuple access that a heap UPDATE must do anyway to lock and outdate
the target tuple.  Another example of a possible win is Andres'
comment upthread that a columnar-storage AM would really rather
deal only with the modified columns.  Also, the patch as it stands
asks FDWs to supply all columns in a whole-row junk var, which is
something that might become unnecessary.

However, there are big stumbling blocks in the way.  ModifyTable
is responsible for applying CHECK constraints, which may require
looking at the values of not-modified columns.  An even bigger
problem is that per-row triggers currently expect to be given
the whole proposed row (and to be able to change columns not
already marked for update).  We could imagine redefining the
trigger APIs to reduce the overhead here, but that's certainly
not going to happen in time for v14.

So for now I think we have to stick with Amit's design of
reconstructing the full updated tuple at the outset in
ModifyTable, and then proceeding pretty much as updates
have done in the past.  But there's more we can do later.

regards, tom lane

Re: truncating timestamps on arbitrary intervals

[Peter Eisentraut]

On 24.03.21 18:25, Erik Rijkers wrote:

On 2021.03.24. 16:38 Peter Eisentraut  wrote:




Committed.



'In cases full units' seems strange.


fixed, thanks

Re: PoC/WIP: Extended statistics on expressions

[Alvaro Herrera]

On 2021-Mar-24, Justin Pryzby wrote:

> On Wed, Mar 24, 2021 at 05:15:46PM +, Dean Rasheed wrote:

> > Hmm, I think "univariate" and "multivariate" are pretty ubiquitous,
> > when used to describe statistics. You could use "single-column" and
> > "multi-column", but then "column" isn't really right anymore, since it
> > might be a column or an expression. I can't think of any other terms
> > that fit.

Agreed.  If we need to define the term, we can spend a sentence or two
in that.

> We already use "multivariate", just not in create-statistics.sgml

> So I think the answer is for create-statistics to expose that word in a
> user-facing way in its reference to multivariate-statistics-examples.

+1

-- 
Álvaro Herrera   Valdivia, Chile