Re: [HACKERS] Creating backup history files for backups taken from standbys

[Michael Paquier]

On Fri, Jan 5, 2018 at 11:27 PM, Simon Riggs  wrote:
> I'm not.
>
> If we want to do this why not only do it in the modes that have meaning?
> i.e. put an if() test in for archive_mode == always.

OK, I can see value in your point as well. The check is a bit more
complicated that just looking for archive_mode == always though, you
need to look at if the backup has been started in recovery or not, and
then adapt using XLogArchivingActive() and XLogArchivingAlways(),
similarly to what is done when waiting for the archives.

> Which also makes it a smaller and clearer patch

Yes, this generates less diffs, reducing the likelihood of bugs. What
do you think about the v3 attached?
-- 
Michael


standby-backup-history-v3.patch
Description: Binary data

Re: [HACKERS] Removing useless DISTINCT clauses

[David Rowley]

Hi Jeff,

Thanks for looking at the patch.

On 6 January 2018 at 10:34, Jeff Janes  wrote:
> Couldn't the Unique node be removed entirely?  If k is a primary key, you
> can't have duplicates in need of removal.

It probably could be, if there were no joins, but any join could
duplicate those rows, so we'd only be able to do that if there were no
joins, and I'm not sure we'd want to special case that. It seems just
too naive to be taken seriously. We could do a lot better...

> Or would that be a subject for a different patch?

Yeah, I'd rather do that fix properly, and do have ideas on how to,
just it's a lot more work (basically testing if the joins can
duplicate rows or not (unique joins++)).

> I think remove_functionally_dependant_groupclauses should have a more
> generic name, like remove_functionally_dependant_clauses.

It's been a while since I looked at this. I remember thinking
something similar at the time but I must have not changed it.

I'll look again and post an updated patch.

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

Re: [HACKERS] Removing useless DISTINCT clauses

[David Rowley]

On 6 January 2018 at 23:08, David Rowley  wrote:
>> I think remove_functionally_dependant_groupclauses should have a more
>> generic name, like remove_functionally_dependant_clauses.
>
> It's been a while since I looked at this. I remember thinking
> something similar at the time but I must have not changed it.
>
> I'll look again and post an updated patch.

I've attached a patch that renames the function as you've suggested.

Thanks again for the review.

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


remove_useless_distinct_clauses_v2.patch
Description: Binary data

Re: FOR EACH ROW triggers on partitioned tables

[Simon Riggs]

On 3 January 2018 at 03:12, Peter Eisentraut
 wrote:
> On 12/29/17 17:53, Alvaro Herrera wrote:
>> This patch enables FOR EACH ROW triggers on partitioned tables.
>>
>> As presented, this patch is sufficient to discuss the semantics that we
>> want for triggers on partitioned tables, which is the most pressing
>> question here ISTM.
>
> This seems pretty straightforward.  What semantics questions do you have?

I see the patch imposes these restrictions

* AFTER TRIGGERS only

* No transition tables

* No WHEN clause

All of which might be removed/extended at some later date

So that's all good... there's not much here, so easy to commit soon.

>> However, this is incomplete: it doesn't create triggers when you do
>> ALTER TABLE ATTACH PARTITION or by CREATE TABLE PARTITION OF.  I'm using
>> this as a basis on which to try foreign keys for partitioned tables.
>> Getting this to committable status requires adding those features.
>
> Yeah that, and also perhaps preventing the removal of triggers from
> partitions if they are supposed to be on the whole partition hierarchy.

+1

> And then make pg_dump do the right things.  That's all mostly legwork, I
> think.
>
> Also, does ALTER TABLE ... ENABLE/DISABLE TRIGGER do the right things on
> partitioned tables?

Not sure I care about that, since it just breaks FKs and other things,
but we can add it later.

-- 
Simon Riggshttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] Removing [Merge]Append nodes which contain a single subpath

[David Rowley]

On 28 December 2017 at 18:31, David Rowley  wrote:
> I've attached a rebased patch.
>
> The previous patch was conflicting with parallel Hash Join (180428404)

And another one. Thanks to the patch tester [1], I realised that I
didn't make check-world and there was a postgres_fdw test that was
failing.

The attached fixes.

[1] http://commitfest.cputube.org/

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


remove_singleton_appends_2018-01-07.patch
Description: Binary data

Re: unique indexes on partitioned tables

[Simon Riggs]

On 29 December 2017 at 23:06, Alvaro Herrera  wrote:
> This is the patch series for UNIQUE / PRIMARY KEY indexes on partitioned
> tables.  This is on top of the patch in
> https://postgr.es/m/20171229175930.3aew7lzwd5w6m2x6@alvherre.pgsql
> but I included it here as 0001 for simplicity.  (Don't review that patch
> in this thread please).  This is essentially the same patch I posted
> elsewhere in that thread.

This is a very simple patch, so not much to object to. Most of the
code is about passing the details thru APIs.

Looks good to go.

The comments are slightly better than the explanation in the docs.

> I included Amit's support for ON CONFLICT DO UPDATE, but as I mentioned
> in the other thread, it has a small bug.  In principle we could push
> 0002 together with 0003, but I'd rather fix 0004 first and push it all
> as one commit.

I agree we want 0004 also, but it would be simpler to just push 0002
and 0003 now and come back later for 0004. That would also avoid any
confusion over patch credits.

> This serves as basis to build foreign keys on top; I'll post that
> separately.

-- 
Simon Riggshttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [PATCH] Logical decoding of TRUNCATE

[Marco Nenciarini]

Patch rebased on the current master.

Regards,
Marco

-- 
Marco Nenciarini - 2ndQuadrant Italy
PostgreSQL Training, Services and Support
marco.nenciar...@2ndquadrant.it | www.2ndQuadrant.it
diff --git a/contrib/test_decoding/expected/ddl.out 
b/contrib/test_decoding/expected/ddl.out
index 1e22c1eefc..d1feea4909 100644
*** a/contrib/test_decoding/expected/ddl.out
--- b/contrib/test_decoding/expected/ddl.out
***
*** 543,548  UPDATE table_with_unique_not_null SET data = 3 WHERE data = 2;
--- 543,550 
  UPDATE table_with_unique_not_null SET id = -id;
  UPDATE table_with_unique_not_null SET id = -id;
  DELETE FROM table_with_unique_not_null WHERE data = 3;
+ TRUNCATE table_with_unique_not_null;
+ TRUNCATE table_with_unique_not_null, table_with_unique_not_null RESTART 
IDENTITY CASCADE;
  -- check toast support
  BEGIN;
  CREATE SEQUENCE toasttable_rand_seq START 79 INCREMENT 1499; -- portable 
"random"
***
*** 660,665  SELECT data FROM 
pg_logical_slot_get_changes('regression_slot', NULL, NULL, 'inc
--- 662,673 
   table public.table_with_unique_not_null: DELETE: id[integer]:4
   COMMIT
   BEGIN
+  table public.table_with_unique_not_null: TRUNCATE: (no-flags)
+  COMMIT
+  BEGIN
+  table public.table_with_unique_not_null: TRUNCATE: restart_seqs cascade
+  COMMIT
+  BEGIN
   table public.toasttable: INSERT: id[integer]:1 
toasted_col1[text]:'12345678910111213141516171819202122232425262728293031323334353637383940414243444546474849505152535455565758596061626364656667686970717273747576777879808182838485868788899091929394959697989910010110210310410510610710810911012113114115116117118119120121122123124125126127128129130131132133134135136137138139140141142143144145146147148149150151152153154155156157158159160161162163164165166167168169170171172173174175176177178179180181182183184185186187188189190191192193194195196197198199200201202203204205206207208209210211212213214215216217218219220221232242252262272282292302312322332342352362372382392402412422432442452462472482492502512522532542552562572582592602612622632642652662672682692702712722732742752762772782792802812822832842852862872882892902912922932942952962972982993003013023033043053063073083093103113123133143153163173183193203213223233243253263273283293303313323433533633733833934034134234334434534634734834935035135235335435535635735835936036136236336436536636736836937037137237337437537637737837938038138238338438538638738838939039139239339439539639739839940040140240340440540640740840941041141241341441541641741841942042142242342442542642742842943043143243343443543643743843944044144244345446447448449450451452453454455456457458459460461462463464465466467468469470471472473474475476477478479480481482483484485486487488489490491492493494495496497498499500501502503504505506507508509510511512513514515516517518519520521522523524525526527528529530531532533534535536537538539540541542543544545546547548549550551552553554565575585595605615625635645655665675685695705715725735745755765775785795805815825835845855865875885895905915925935945955965975985996006016026036046056066076086096106116126136146156166176186196206216226236246256266276286296306316326336346356366376386396406416426436446456466476486496506516526536546556566576586596606616626636646656766866967067167267367467567667767867968068168268368468568668768868969069169269369469569669769869970070170270370470570670770870971071171271371471571671771871972072172272372472572672772872973073173273373473573673773873974074174274374474574674774874975075175275375475575675775875976076176276376476576676776876977077177277377477577678779780781782783784785786787788789790791792793794795796797798799800801802803804805806807808809810811812813814815816817818819820821822823824825826827828829830831832833834835836837838839840841842843844845846847848849850851852853854855856857858859860861862863864865866867868869870871872873874875876877878879880881882883884885886887898908918928938948958968978988999009019029039049059069079089099109119129139149159169179189199209219229239249259269279289299309319329339349359369379389399409419429439449459469479489499509519529539549559569579589599609619629639649659669679689699709719729739749759769779789799809819829839849859869879889899909919929939949959969979989991000100110021003100410051006100710081009101010111012101310141015101610171018101910201021102210231024102510261027102810291030103110321033103410351036103710381039104010411042104310441045104610471048104910501051105210531054105510561057105810591060106110621063106410651066106710681069107010711072107310741075107610771078107910801081108210831084108510861087108810891090109110921093109410951096109710981099110011011102110311041105110611071108110911101112111311141115111611171118111911201121112211231124112511261127112811291130113111321133113411351136113711381139114011411142114311441145114611471148114911501151115211531154115511561157115811591160116111621163116411651166116711681169117011711172117311741175117611771

Re: to_timestamp TZH and TZM format specifiers

[Andrew Dunstan]

On 01/03/2018 02:21 PM, Andrew Dunstan wrote:
>
> On 01/03/2018 01:34 PM, Tom Lane wrote:
>> Andrew Dunstan  writes:
>>> This small and simple standalone patch extracted from the SQL/JSON work
>>> would allow the user to supply a string with a time zone specified as
>>> hh:mm thus:
>>> SELECT to_timestamp('2011-12-18 11:38 -05:20', '-MM-DD HH12:MI
>>> TZH:TZM');
>>>  to_timestamp
>>> --
>>>  Sun Dec 18 08:58:00 2011 PST
>> I see that Oracle's to_timestamp supports these format codes, so +1
>> if you've checked that the behavior is compatible with Oracle.  The
>> most obvious possible gotcha is whether + is east or west of GMT,
>> but also there's formatting questions like what the field width is
>> and whether leading zeroes are printed.
>>
>> Also, I'm unimpressed that you've not bothered to implement the
>> to_char direction.  That moves this from a feature addition to
>> a kluge, IMO, especially since that ought to be the easier direction.
>>
>>
>
>
> To be clear, this isn't my patch, it one I extracted from the large
> patchset Nikita Glukhov posted for SQL/JSON, in order to kickstart
> process there.
>
> I wasn't aware of the Oracle implementation.
>
> I agree that supporting these in to_char would be useful, and should not
> be terribly difficult.
>
>


Here is a version that adds the to_char direction. AFAICT it is
compatible with Oracle.

cheers

andrew

-- 

Andrew Dunstanhttps://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 4dd9d02..2428434 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -6074,6 +6074,14 @@ SELECT regexp_match('abc01234xyz', '(?:(.*?)(\d+)(.*)){1,1}');
  (only supported in to_char)


+   TZH
+time-zone hours
+   
+   
+   TZM
+time-zone minutes
+   
+   
 OF
 time-zone offset from UTC
  (only supported in to_char)
diff --git a/src/backend/utils/adt/formatting.c b/src/backend/utils/adt/formatting.c
index 0e30810..b8bd4ca 100644
--- a/src/backend/utils/adt/formatting.c
+++ b/src/backend/utils/adt/formatting.c
@@ -424,7 +424,10 @@ typedef struct
 j,
 us,
 yysz,			/* is it YY or  ? */
-clock;			/* 12 or 24 hour clock? */
+clock,			/* 12 or 24 hour clock? */
+tzsign,			/* +1, -1 or 0 if timezone info is absent */
+tzh,
+tzm;
 } TmFromChar;
 
 #define ZERO_tmfc(_X) memset(_X, 0, sizeof(TmFromChar))
@@ -470,6 +473,7 @@ do {	\
 	(_X)->tm_sec  = (_X)->tm_year = (_X)->tm_min = (_X)->tm_wday = \
 	(_X)->tm_hour = (_X)->tm_yday = (_X)->tm_isdst = 0; \
 	(_X)->tm_mday = (_X)->tm_mon  = 1; \
+	(_X)->tm_zone = NULL; \
 } while(0)
 
 #define ZERO_tmtc(_X) \
@@ -609,6 +613,8 @@ typedef enum
 	DCH_RM,
 	DCH_,
 	DCH_SS,
+	DCH_TZH,
+	DCH_TZM,
 	DCH_TZ,
 	DCH_US,
 	DCH_WW,
@@ -756,7 +762,9 @@ static const KeyWord DCH_keywords[] = {
 	{"RM", 2, DCH_RM, false, FROM_CHAR_DATE_GREGORIAN}, /* R */
 	{"", 4, DCH_, true, FROM_CHAR_DATE_NONE},	/* S */
 	{"SS", 2, DCH_SS, true, FROM_CHAR_DATE_NONE},
-	{"TZ", 2, DCH_TZ, false, FROM_CHAR_DATE_NONE},	/* T */
+	{"TZH", 3, DCH_TZH, false, FROM_CHAR_DATE_NONE},	/* T */
+	{"TZM", 3, DCH_TZM, true, FROM_CHAR_DATE_NONE},
+	{"TZ", 2, DCH_TZ, false, FROM_CHAR_DATE_NONE},
 	{"US", 2, DCH_US, true, FROM_CHAR_DATE_NONE},	/* U */
 	{"WW", 2, DCH_WW, true, FROM_CHAR_DATE_GREGORIAN},	/* W */
 	{"W", 1, DCH_W, true, FROM_CHAR_DATE_GREGORIAN},
@@ -879,7 +887,7 @@ static const int DCH_index[KeyWord_INDEX_SIZE] = {
 	-1, -1, -1, -1, -1, -1, -1, -1, -1, -1,
 	-1, -1, -1, -1, -1, DCH_A_D, DCH_B_C, DCH_CC, DCH_DAY, -1,
 	DCH_FX, -1, DCH_HH24, DCH_IDDD, DCH_J, -1, -1, DCH_MI, -1, DCH_OF,
-	DCH_P_M, DCH_Q, DCH_RM, DCH_, DCH_TZ, DCH_US, -1, DCH_WW, -1, DCH_Y_YYY,
+	DCH_P_M, DCH_Q, DCH_RM, DCH_, DCH_TZH, DCH_US, -1, DCH_WW, -1, DCH_Y_YYY,
 	-1, -1, -1, -1, -1, -1, -1, DCH_a_d, DCH_b_c, DCH_cc,
 	DCH_day, -1, DCH_fx, -1, DCH_hh24, DCH_iddd, DCH_j, -1, -1, DCH_mi,
 	-1, -1, DCH_p_m, DCH_q, DCH_rm, DCH_, DCH_tz, DCH_us, -1, DCH_ww,
@@ -2519,6 +2527,19 @@ DCH_to_char(FormatNode *node, bool is_interval, TmToChar *in, char *out, Oid col
 	s += strlen(s);
 }
 break;
+			case DCH_TZH:
+INVALID_FOR_INTERVAL;
+sprintf(s, "%c%02d",
+		(tm->tm_gmtoff >= 0) ? '+' : '-',
+		abs((int) tm->tm_gmtoff) / SECS_PER_HOUR);
+s += strlen(s);
+break;
+			case DCH_TZM:
+INVALID_FOR_INTERVAL;
+sprintf(s, "%02d",
+		(abs((int) tm->tm_gmtoff) % SECS_PER_HOUR) / SECS_PER_MINUTE);
+s += strlen(s);
+break;
 			case DCH_OF:
 INVALID_FOR_INTERVAL;
 sprintf(s, "%c%0*d",
@@ -3070,6 +3091,20 @@ DCH_from_char(FormatNode *node, char *in, TmFromChar *out)
 		 errmsg("formatting field \"%s\" is only supported in to_char",
 n->key->name)));
 break;
+			case DCH_TZH:
+out->tzsign = *s =

Re: [HACKERS] Creating backup history files for backups taken from standbys

[David Steele]

On 1/6/18 3:48 AM, Michael Paquier wrote:

On Fri, Jan 5, 2018 at 11:27 PM, Simon Riggs  wrote:


Which also makes it a smaller and clearer patch


Yes, this generates less diffs, reducing the likelihood of bugs. What
do you think about the v3 attached?


I agree that this is a cleaner solution.

--
-David
da...@pgmasters.net

Re: Challenges preventing us moving to 64 bit transaction id (XID)?

[Ryan Murphy]

Since the Patch Tester (http://commitfest.cputube.org/) says this Patch will 
not apply correctly, I am tempted to change the status to Waiting on Author.

However, I'm new to the CommitFest process, so I'm leaving the status as-is for 
now and asking Stephen Frost whether he agrees.

I haven't tried to apply the patch myself yet, but happy to do so if e.g. we 
think the Patch Tester can't be taken on face value,
or if I need to find specific feedback about why the patch didn't apply to help 
the Author.

Best regards,
Ryan

Re: Rangejoin rebased

[Simon Riggs]

On 29 December 2017 at 18:25, Jeff Davis  wrote:
> New rangejoin patch attached.
>
> I had previously attempted to make this work well for multiple range
> join keys, but this patch implements single rangejoin keys only, and
> the rest of the rangejoin clauses are effectively just rechecks. I
> believe it can be made effective for multiple rangejoin keys, but at
> the cost of additional complexity which is neither justified nor
> implemented at this point.

For this to be useful, it needs to include some details of how to use
it when people have NOT used range datatypes in their tables.

If we can see some examples with StartDate and EndDate cast to a
tzrange, plus some "don't write it like this" anti-patterns that would
help.

We can then make it clear that this is a huge performance benefit for
these important cases.

Just to emphasise why we want this, it might be better for the EXPLAIN
to say "Time Range Join" when the ranges being joined are Time Ranges,
and for other cases to just say "Range Join". The use of the word
Merge doesn't help much there.

-- 
Simon Riggshttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Challenges preventing us moving to 64 bit transaction id (XID)?

[Ryan Murphy]

Thanks for this contribution!
I think it's a hard but important problem to upgrade these xids.

Unfortunately, I've confirmed that this patch 0001-64bit-guc-relopt-3.patch 
doesn't apply correctly on my computer.

Here's what I did:

I did a "git pull" to the current HEAD, which is 
6271fceb8a4f07dafe9d67dcf7e849b319bb2647

Then I attempted to apply the patch, here's what I saw:

$ git apply patches/0001-64bit-guc-relopt-3.patch 
error: src/backend/access/common/reloptions.c: already exists in working 
directory
error: src/backend/utils/misc/guc.c: already exists in working directory
error: src/include/access/reloptions.h: already exists in working directory
error: src/include/utils/guc.h: already exists in working directory
error: src/include/utils/guc_tables.h: already exists in working directory

Alexander, what is the process you're using to create the patch?  I've heard 
someone (maybe Tom Lane?) say that he sometimes uses "patch" directly instead 
of "git" to create the patch, with better results.  I forget the exact command.

For now I'm setting this to Waiting on Author, until we have a patch that 
applies via "git apply".

Thanks!
Ryan

The new status of this patch is: Waiting on Author

Re: [PATCH] Logical decoding of TRUNCATE

[Marco Nenciarini]

Attached here there is the complete list of patches required to pass all
the tests. The 0001 patch is discussed in a separate thread, but I've
posted it also here to ease the review of the 0002.

Regards,
Marco

-- 
Marco Nenciarini - 2ndQuadrant Italy
PostgreSQL Training, Services and Support
marco.nenciar...@2ndquadrant.it | www.2ndQuadrant.it
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index e4a01699e4..aff56891e6 100644
*** a/doc/src/sgml/config.sgml
--- b/doc/src/sgml/config.sgml
***
*** 6502,6508  COPY postgres_log FROM '/full/path/to/logfile.csv' WITH csv;

 
  Controls firing of replication-related triggers and rules for the
! current session.  Setting this variable requires
  superuser privilege and results in discarding any previously cached
  query plans.  Possible values are origin (the 
default),
  replica and local.
--- 6502,6510 

 
  Controls firing of replication-related triggers and rules for the
! current session. When set to replica it also
! disables all the foreign key checks, which can leave the data in an
! inconsistent state if improperly used. Setting this variable requires
  superuser privilege and results in discarding any previously cached
  query plans.  Possible values are origin (the 
default),
  replica and local.
diff --git a/src/backend/commanindex d979ce266d..296807849f 100644
*** a/src/backend/commands/tablecmds.c
--- b/src/backend/commands/tablecmds.c
***
*** 1341,1356  ExecuteTruncate(TruncateStmt *stmt)
}
  
/*
!* Check foreign key references.  In CASCADE mode, this should be
!* unnecessary since we just pulled in all the references; but as a
!* cross-check, do it anyway if in an Assert-enabled build.
 */
  #ifdef USE_ASSERT_CHECKING
-   heap_truncate_check_FKs(rels, false);
- #else
-   if (stmt->behavior == DROP_RESTRICT)
heap_truncate_check_FKs(rels, false);
  #endif
  
/*
 * If we are asked to restart sequences, find all the sequences, lock 
them
--- 1341,1364 
}
  
/*
!* Suppress foreign key references check if session replication role is
!* set to REPLICA.
 */
+   if (SessionReplicationRole != SESSION_REPLICATION_ROLE_REPLICA)
+   {
+ 
+   /*
+* Check foreign key references.  In CASCADE mode, this should 
be
+* unnecessary since we just pulled in all the references; but 
as a
+* cross-check, do it anyway if in an Assert-enabled build.
+*/
  #ifdef USE_ASSERT_CHECKING
heap_truncate_check_FKs(rels, false);
+ #else
+   if (stmt->behavior == DROP_RESTRICT)
+   heap_truncate_check_FKs(rels, false);
  #endif
+   }
  
/*
 * If we are asked to restart sequences, find all the sequences, lock 
them
diff --git a/contrib/test_decoding/expected/ddl.out 
b/contrib/test_decoding/expected/ddl.out
index 1e22c1eefc..d1feea4909 100644
*** a/contrib/test_decoding/expected/ddl.out
--- b/contrib/test_decoding/expected/ddl.out
***
*** 543,548  UPDATE table_with_unique_not_null SET data = 3 WHERE data = 2;
--- 543,550 
  UPDATE table_with_unique_not_null SET id = -id;
  UPDATE table_with_unique_not_null SET id = -id;
  DELETE FROM table_with_unique_not_null WHERE data = 3;
+ TRUNCATE table_with_unique_not_null;
+ TRUNCATE table_with_unique_not_null, table_with_unique_not_null RESTART 
IDENTITY CASCADE;
  -- check toast support
  BEGIN;
  CREATE SEQUENCE toasttable_rand_seq START 79 INCREMENT 1499; -- portable 
"random"
***
*** 660,665  SELECT data FROM 
pg_logical_slot_get_changes('regression_slot', NULL, NULL, 'inc
--- 662,673 
   table public.table_with_unique_not_null: DELETE: id[integer]:4
   COMMIT
   BEGIN
+  table public.table_with_unique_not_null: TRUNCATE: (no-flags)
+  COMMIT
+  BEGIN
+  table public.table_with_unique_not_null: TRUNCATE: restart_seqs cascade
+  COMMIT
+  BEGIN
   table public.toasttable: INSERT: id[integer]:1 
toasted_col1[text]:'123456789101112131415161718192021222324252627282930313233343536373839404142434445464748495051525354555657585960616263646566676869707172737475767778798081828384858687888990919293949596979899100101102103104105106107108109110121131141151161171181191201211221231241251261271281291301311321331341351361371381391401411421431441451461471481491501511521531541551561571581591601611621631641651661671681691701711721731741751761771781791801811821831841851861871881891901911921931941951961971981992002012022032042052062072082092102112122132142152162172182192202212322422522622722822923023123223323423523623723823924024124224324424524624724824925025125225325425525625725825926026126226326426526626726826927027127227327427527627727827928028128228328

Re: Add default role 'pg_access_server_files'

[Ryan Murphy]

Stephen, so far I've read thru your patch and familiarized myself with some of 
the auth functionality in pg_authid.h and src/backend/utils/adt/acl.c

The only question I have so far about your patch is the last several hunks of 
the diff, which remove superuser checks without adding anything immediately 
obvious in their place:

...
@@ -195,11 +205,6 @@ pg_read_file(PG_FUNCTION_ARGS)
char   *filename;
text   *result;
 
-   if (!superuser())
-   ereport(ERROR,
-   (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
-(errmsg("must be superuser to read files";
-
/* handle optional arguments */
if (PG_NARGS() >= 3)
{
@@ -236,11 +241,6 @@ pg_read_binary_file(PG_FUNCTION_ARGS)
char   *filename;
bytea  *result;
 
-   if (!superuser())
-   ereport(ERROR,
-   (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
-(errmsg("must be superuser to read files";
-
/* handle optional arguments */
if (PG_NARGS() >= 3)
{
@@ -313,11 +313,6 @@ pg_stat_file(PG_FUNCTION_ARGS)
TupleDesc   tupdesc;
boolmissing_ok = false;
 
-   if (!superuser())
-   ereport(ERROR,
-   (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
-(errmsg("must be superuser to get file information";
-
/* check the optional argument */
if (PG_NARGS() == 2)
missing_ok = PG_GETARG_BOOL(1);
...

I wanted to ask if you have reason to believe that these checks were not 
necessary (and therefore can be deleted instead of replaced by 
is_member_of_role() checks like you did elsewhere).  I still have limited 
understanding of the overall code, so really just asking because it's the first 
thing that jumped out.

Best,
Ryan

Re: [HACKERS] [PROPOSAL] Temporal query processing with range types

[Simon Riggs]

On 30 November 2017 at 17:26, Robert Haas  wrote:

> I wonder if we could instead think about R NORMALIZE S ON R.x = S.y
> WITH (R.time, S.time) as an ordinary join on R.x = S.y with some extra
> processing afterwards.

That would work nicely, kindof like a Projection, but one that can
vary the number of rows emitted.

For normal joins, we simply emit one row. For new style joins we call
a special PostJoinSetProjection function: one tuple in, potentially
many tuples out.

Peter, does Robert's proposed treatment give you what you need?


Overall, I like the goals expressed on this thread. I think if we
should focus on introducing useful new functionality, rather than
focusing on syntax.

I'm not very keen on adopting new syntax that isn't in the
SQLStandard. They have a bad habit of doing something completely
different. So a flexible approach will allow us to have functionality
now and we can adopt any new syntax later.

For any new syntax, I think the right approach would be to create a
new parser plugin. That way we could make all of this part of an
extension.
* a parser plugin for any new syntax
* various PostJoinSetProjection() functions to be called as needed
So the idea is we enable Postgres to allow major new functionality, as
was done for PostGIS so successfully.

We can adopt syntax into the main parser later once SQLStandard
accepts this, or some munged version of it.

-- 
Simon Riggshttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Add default role 'pg_access_server_files'

[Stephen Frost]

Greetings Ryan!

* Ryan Murphy (ryanfmur...@gmail.com) wrote:
> Stephen, so far I've read thru your patch and familiarized myself with some 
> of the auth functionality in pg_authid.h and src/backend/utils/adt/acl.c
> 
> The only question I have so far about your patch is the last several hunks of 
> the diff, which remove superuser checks without adding anything immediately 
> obvious in their place:

Ah, I realize it's not immediately obvious, but they *are* replaced by
something else- REVOKE statements in the "system_views.sql" file in
src/backend/catalog:

REVOKE EXECUTE ON FUNCTION pg_read_file(text) FROM public;
REVOKE EXECUTE ON FUNCTION pg_read_file(text,bigint,bigint) FROM public;
REVOKE EXECUTE ON FUNCTION pg_read_file(text,bigint,bigint,boolean) FROM public;

REVOKE EXECUTE ON FUNCTION pg_read_binary_file(text) FROM public;
REVOKE EXECUTE ON FUNCTION pg_read_binary_file(text,bigint,bigint) FROM public;
REVOKE EXECUTE ON FUNCTION pg_read_binary_file(text,bigint,bigint,boolean) FROM 
public;

REVOKE EXECUTE ON FUNCTION pg_stat_file(text) FROM public;
REVOKE EXECUTE ON FUNCTION pg_stat_file(text,boolean) FROM public;

That script is run as part of 'initdb' to set things up in the system.
By issueing those REVOKE statements, no one but the cluster owner (a
superuser) is able to run those functions- unless a superuser decides
that it's ok for others to run them, in which case they would run:

GRANT EXECUTE ON FUNCTION pg_read_file(text) TO myuser;

> I wanted to ask if you have reason to believe that these checks were not 
> necessary (and therefore can be deleted instead of replaced by 
> is_member_of_role() checks like you did elsewhere).  I still have limited 
> understanding of the overall code, so really just asking because it's the 
> first thing that jumped out.

The places where is_member_of_role() is being used are cases where it's
not possible to use the GRANT system.  For example, we don't have a way
to say "GRANT read-files-outside-the-data-directory TO role1;" in the
normal GRANT system, and so a default role is added to allow that
specific right to be GRANT'd to non-superuser.

One would need to have both the default role AND EXECUTE rights on the
function to be able to, say, run:

SELECT pg_read_file('/data/load_area/load_file');

With just EXECUTE on the function, they could use pg_read_file() to read
files under the data directory but not elsewhere on the system, which
may be overly limiting for some use-cases.

Of course, all of these functions allow a great deal of access to the
system and that's why they started out being superuser-only.
Administrators will need to carefully consider who, if anyone, should
have the level of access which these functions and default roles
provide.

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: Condition variable live lock

[Tom Lane]

I wrote:
> Robert Haas  writes:
>> No opinion without seeing what you propose to change.

> OK, will put out a proposal.

I began with the intention of making no non-cosmetic changes, but then
I started to wonder why ConditionVariablePrepareToSleep bothers with a
!proclist_contains test, when the calling process surely ought not be
in the list -- or any other list -- since it wasn't prepared to sleep.
And that led me down a different rabbit hole ending in the conclusion
that proclist.h could stand some improvements too.  I do not like the
fact that it's impossible to tell whether a proclist_node is in any
proclist or not.  Initially, a proclist_node contains zeroes which is
a distinguishable state, but proclist_delete_offset resets it to
next = prev = INVALID_PGPROCNO which looks the same as a node that's in a
singleton list.  We should have it reset to the initial state of zeroes
instead, and then we can add assertions to proclist_push_xxx that the
supplied node is not already in a list.  Hence, I propose the first
attached patch which tightens things up in proclist.h and then removes
the !proclist_contains test in ConditionVariablePrepareToSleep; the
assertion in proclist_push_tail supersedes that.

The second attached patch is the cosmetic changes I want to make in
condition_variable.c/.h.

I still think that we ought to change the Asserts on cv_sleep_target in
ConditionVariablePrepareToSleep and ConditionVariableSleep to be full
test-and-elog tests.  Those are checking a global correctness property
("global" meaning "interactions between completely unrelated modules can
break this"), and they'd be extremely cheap compared to the rest of what
those functions are doing, so I think insisting that they be Asserts is
penny wise and pound foolish.  Anybody besides Robert want to vote on
that?

Another loose end that I'm seeing here is that while a process waiting on
a condition variable will respond to a cancel or die interrupt, it will
not notice postmaster death.  This seems unwise to me.  I think we should
adjust the WaitLatch call to include WL_POSTMASTER_DEATH as a wake
condition and just do a summary proc_exit(1) if it sees that.  I'd even
argue that that is a back-patchable bug fix.

regards, tom lane

diff --git a/src/include/storage/proclist_types.h b/src/include/storage/proclist_types.h
index 237fb76..f4dac10 100644
--- a/src/include/storage/proclist_types.h
+++ b/src/include/storage/proclist_types.h
@@ -16,7 +16,12 @@
 #define PROCLIST_TYPES_H
 
 /*
- * A node in a list of processes.
+ * A node in a doubly-linked list of processes.  The link fields contain
+ * the 0-based PGPROC indexes of the next and previous process, or
+ * INVALID_PGPROCNO in the next-link of the last node and the prev-link
+ * of the first node.  A node that is currently not in any list
+ * should have next == prev == 0; this is not a possible state for a node
+ * that is in a list, because we disallow circularity.
  */
 typedef struct proclist_node
 {
@@ -25,7 +30,8 @@ typedef struct proclist_node
 } proclist_node;
 
 /*
- * Head of a doubly-linked list of PGPROCs, identified by pgprocno.
+ * Header of a doubly-linked list of PGPROCs, identified by pgprocno.
+ * An empty list is represented by head == tail == INVALID_PGPROCNO.
  */
 typedef struct proclist_head
 {
@@ -42,4 +48,4 @@ typedef struct proclist_mutable_iter
 	int			next;			/* pgprocno of the next PGPROC */
 } proclist_mutable_iter;
 
-#endif
+#endif			/* PROCLIST_TYPES_H */
diff --git a/src/include/storage/proclist.h b/src/include/storage/proclist.h
index 4e25b47..59a478e 100644
--- a/src/include/storage/proclist.h
+++ b/src/include/storage/proclist.h
@@ -42,7 +42,7 @@ proclist_is_empty(proclist_head *list)
 
 /*
  * Get a pointer to a proclist_node inside a given PGPROC, given a procno and
- * an offset.
+ * the proclist_node field's offset within struct PGPROC.
  */
 static inline proclist_node *
 proclist_node_get(int procno, size_t node_offset)
@@ -53,13 +53,15 @@ proclist_node_get(int procno, size_t node_offset)
 }
 
 /*
- * Insert a node at the beginning of a list.
+ * Insert a process at the beginning of a list.
  */
 static inline void
 proclist_push_head_offset(proclist_head *list, int procno, size_t node_offset)
 {
 	proclist_node *node = proclist_node_get(procno, node_offset);
 
+	Assert(node->next == 0 && node->prev == 0);
+
 	if (list->head == INVALID_PGPROCNO)
 	{
 		Assert(list->tail == INVALID_PGPROCNO);
@@ -79,13 +81,15 @@ proclist_push_head_offset(proclist_head *list, int procno, size_t node_offset)
 }
 
 /*
- * Insert a node at the end of a list.
+ * Insert a process at the end of a list.
  */
 static inline void
 proclist_push_tail_offset(proclist_head *list, int procno, size_t node_offset)
 {
 	proclist_node *node = proclist_node_get(procno, node_offset);
 
+	Assert(node->next == 0 && node->prev == 0);
+
 	if (list->tail == INVALID_PGPROCNO)
 	{
 		Assert(list->head == INVALID_PGPROCNO);
@@ -105,30 +109,38 @@ procli

Re: [HACKERS] SQL/JSON in PostgreSQL

[Oleg Bartunov]

On Sat, Jan 6, 2018 at 8:22 AM, Pavel Stehule  wrote:
> Hi
>
> I am checking the JSONPath related code
>
> Questions, notes:
>
> 1. jsonpath operators are not consistent with any other .. json, xml .. I am
> missing ?, @> operátors

I have slides about jsonpath
http://www.sai.msu.su/~megera/postgres/talks/sqljson-pgconf.eu-2017.pdf

> 2. documentation issue - there is "'{"a":[1,2,3,4,5]}'::json *? '$.a[*] ? (@
>> 2)'" - operator *? doesn't exists

There are should be @? operator

> 3. operator @~ looks like too aggressive shortcut - should be better
> commented
>
> What is not clean, if jsonpath should to create some new operators for json,
> jsonb types? It is special filter, defined by type, so from my perspective
> the special operators are not necessary.

It's impossible to distinguish jsonpath from text, so introducing new operators
are easier than everytime explicitly specify jsonpath datatype.

>
> Regards
>
> Pavel
>
>

Re: Challenges preventing us moving to 64 bit transaction id (XID)?

[Tom Lane]

Ryan Murphy  writes:
> Alexander, what is the process you're using to create the patch?  I've heard 
> someone (maybe Tom Lane?) say that he sometimes uses "patch" directly instead 
> of "git" to create the patch, with better results.  I forget the exact 
> command.

Nah, you've got that the other way 'round.  "patch" is not for creating
patches, it's for applying them.  I've found, and some other people seem
to agree, that "patch" is more robust at applying patches than "git apply"
is.  You might try this for a patch created with "git diff":

patch -p1 

Re: [HACKERS] SQL/JSON in PostgreSQL

[Pavel Stehule]

2018-01-06 22:02 GMT+01:00 Oleg Bartunov :

> On Sat, Jan 6, 2018 at 8:22 AM, Pavel Stehule 
> wrote:
> > Hi
> >
> > I am checking the JSONPath related code
> >
> > Questions, notes:
> >
> > 1. jsonpath operators are not consistent with any other .. json, xml ..
> I am
> > missing ?, @> operátors
>
> I have slides about jsonpath
> http://www.sai.msu.su/~megera/postgres/talks/sqljson-pgconf.eu-2017.pdf
>
> > 2. documentation issue - there is "'{"a":[1,2,3,4,5]}'::json *? '$.a[*]
> ? (@
> >> 2)'" - operator *? doesn't exists
>
> There are should be @? operator
>
> > 3. operator @~ looks like too aggressive shortcut - should be better
> > commented
> >
> > What is not clean, if jsonpath should to create some new operators for
> json,
> > jsonb types? It is special filter, defined by type, so from my
> perspective
> > the special operators are not necessary.
>
> It's impossible to distinguish jsonpath from text, so introducing new
> operators
> are easier than everytime explicitly specify jsonpath datatype.
>

There are two possible solutions - special operator or explicit casting. In
this case I am not sure if special operator for this case is good solution.
Probably nobody will use it - because there SQL/JSON functions, but I don't
think so this inconsistency is correct.

I have not strong opinion about it - it will be hidden feature for almost
all users.


> >
> > Regards
> >
> > Pavel
> >
> >
>

Re: [HACKERS] SQL/JSON in PostgreSQL

[Pavel Stehule]

Hi

I try jsonpath on json

{
"book":
[
{
"title": "Beginning JSON",
"author": "Ben Smith",
"price": 49.99
},

{
"title": "JSON at Work",
"author": "Tom Marrs",
"price": 29.99
},

{
"title": "Learn JSON in a DAY",
"author": "Acodemy",
"price": 8.99
},

{
"title": "JSON: Questions and Answers",
"author": "George Duckett",
"price": 6.00
}
],

"price range":
{
"cheap": 10.00,
"medium": 20.00
}
}


I am not jsonpath expert, so I can be bad

How I can get title of book with cost 6?

postgres=# select j @* '$.book[*] ? (@.price==6)' from test;
┌─┐
│  ?column?   │
╞═╡
│ {  ↵│
│ "title": "JSON: Questions and Answers",↵│
│ "author": "George Duckett",↵│
│ "price": 6.00  ↵│
│ }  ↵│
│ │
└─┘
(1 row)

-- not sure, if it is correct
postgres=# select j @* '$.book[*].title ? (@.price==6)' from test;
┌──┐
│ ?column? │
╞══╡
└──┘
(0 rows)

I found some examples, where the filter has bigger sense, but it is not
supported


LINE 1: select j @* '$.book[?(@.price==6.00)].title' from test;
^
DETAIL:  syntax error, unexpected '?' at or near "?"

Regards

Pavel

Re: [HACKERS] SQL/JSON in PostgreSQL

[Nikita Glukhov]

On 07.01.2018 00:22, Pavel Stehule wrote:


Hi

I try jsonpath on json

{
    "book":
    [
    {
    "title": "Beginning JSON",
    "author": "Ben Smith",
    "price": 49.99
    },

    {
    "title": "JSON at Work",
    "author": "Tom Marrs",
    "price": 29.99
    },

    {
    "title": "Learn JSON in a DAY",
    "author": "Acodemy",
    "price": 8.99
    },

    {
    "title": "JSON: Questions and Answers",
    "author": "George Duckett",
    "price": 6.00
    }
    ],

    "price range":
    {
    "cheap": 10.00,
    "medium": 20.00
    }
}


I am not jsonpath expert, so I can be bad

How I can get title of book with cost 6?

postgres=# select j @* '$.book[*] ? (@.price==6)' from test;
┌─┐
│  ?column?   │
╞═╡
│ {  ↵│
│ "title": "JSON: Questions and Answers",↵│
│ "author": "George Duckett",    ↵│
│ "price": 6.00  ↵│
│ }  ↵│
│ │
└─┘
(1 row)

-- not sure, if it is correct
postgres=# select j @* '$.book[*].title ? (@.price==6)' from test;
┌──┐
│ ?column? │
╞══╡
└──┘
(0 rows)

I found some examples, where the filter has bigger sense, but it is 
not supported



LINE 1: select j @* '$.book[?(@.price==6.00)].title' from test;
    ^
DETAIL:  syntax error, unexpected '?' at or near "?"


".title" simply should go after the filter:

select j @* '$.book[*] ? (@.price==6.00).title' from test;


--
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: [HACKERS] SQL/JSON in PostgreSQL

[Pavel Stehule]

2018-01-06 22:23 GMT+01:00 Nikita Glukhov :

> On 07.01.2018 00:22, Pavel Stehule wrote:
>
> Hi
>
> I try jsonpath on json
>
> {
> "book":
> [
> {
> "title": "Beginning JSON",
> "author": "Ben Smith",
> "price": 49.99
> },
>
> {
> "title": "JSON at Work",
> "author": "Tom Marrs",
> "price": 29.99
> },
>
> {
> "title": "Learn JSON in a DAY",
> "author": "Acodemy",
> "price": 8.99
> },
>
> {
> "title": "JSON: Questions and Answers",
> "author": "George Duckett",
> "price": 6.00
> }
> ],
>
> "price range":
> {
> "cheap": 10.00,
> "medium": 20.00
> }
> }
>
>
> I am not jsonpath expert, so I can be bad
>
> How I can get title of book with cost 6?
>
> postgres=# select j @* '$.book[*] ? (@.price==6)' from test;
> ┌─┐
> │  ?column?   │
> ╞═╡
> │ {  ↵│
> │ "title": "JSON: Questions and Answers",↵│
> │ "author": "George Duckett",↵│
> │ "price": 6.00  ↵│
> │ }  ↵│
> │ │
> └─┘
> (1 row)
>
> -- not sure, if it is correct
> postgres=# select j @* '$.book[*].title ? (@.price==6)' from test;
> ┌──┐
> │ ?column? │
> ╞══╡
> └──┘
> (0 rows)
>
> I found some examples, where the filter has bigger sense, but it is not
> supported
>
>
> LINE 1: select j @* '$.book[?(@.price==6.00)].title' from test;
> ^
> DETAIL:  syntax error, unexpected '?' at or near "?"
>
> ".title" simply should go after the filter:
>
> select j @* '$.book[*] ? (@.price==6.00).title' from test;
>
>
It is working, thank you.

and the form "$.book[?(@.price==6.00)].title" ? I found this example in
some other SQL/JSON implementations.



> --
> Nikita Glukhov
> Postgres Professional: http://www.postgrespro.com
> The Russian Postgres Company
>

Re: [HACKERS] SQL/JSON in PostgreSQL

[Nikita Glukhov]

On 07.01.2018 00:33, Pavel Stehule wrote:

2018-01-06 22:23 GMT+01:00 Nikita Glukhov >:


On 07.01.2018 00:22, Pavel Stehule wrote:


Hi

I try jsonpath on json

{
    "book":
    [
    {
    "title": "Beginning JSON",
    "author": "Ben Smith",
    "price": 49.99
    },

    {
    "title": "JSON at Work",
    "author": "Tom Marrs",
    "price": 29.99
    },

    {
    "title": "Learn JSON in a DAY",
    "author": "Acodemy",
    "price": 8.99
    },

    {
    "title": "JSON: Questions and Answers",
    "author": "George Duckett",
    "price": 6.00
    }
    ],

    "price range":
    {
    "cheap": 10.00,
    "medium": 20.00
    }
}


I am not jsonpath expert, so I can be bad

How I can get title of book with cost 6?

postgres=# select j @* '$.book[*] ? (@.price==6)' from test;
┌─┐
│ ?column?   │
╞═╡
│ { ↵│
│ "title": "JSON: Questions and Answers",↵│
│ "author": "George Duckett",    ↵│
│ "price": 6.00  ↵│
│ } ↵│
│ │
└─┘
(1 row)

-- not sure, if it is correct
postgres=# select j @* '$.book[*].title ? (@.price==6)' from test;
┌──┐
│ ?column? │
╞══╡
└──┘
(0 rows)

I found some examples, where the filter has bigger sense, but it
is not supported


LINE 1: select j @* '$.book[?(@.price==6.00)].title' from test;
    ^
DETAIL:  syntax error, unexpected '?' at or near "?"


".title" simply should go after the filter:

select j @* '$.book[*] ? (@.price==6.00).title' from test;


It is working, thank you.

and the form "$.book[?(@.price==6.00)].title" ? I found this example 
in some other SQL/JSON implementations.


This is non-standard feature, but it can be easily added for 
compatibility with other implementations.


--
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: Condition variable live lock

[Tom Lane]

I wrote:
> I still think that we ought to change the Asserts on cv_sleep_target in
> ConditionVariablePrepareToSleep and ConditionVariableSleep to be full
> test-and-elog tests.  Those are checking a global correctness property
> ("global" meaning "interactions between completely unrelated modules can
> break this"), and they'd be extremely cheap compared to the rest of what
> those functions are doing, so I think insisting that they be Asserts is
> penny wise and pound foolish.

Actually ... perhaps a better design would be to have
ConditionVariable[PrepareTo]Sleep auto-cancel any prepared sleep for
a different condition variable, analogously to what we just did in
ConditionVariableBroadcast, on the same theory that whenever control
returns to the other CV wait loop it can re-establish the relevant
state easily enough.  I have to think that if the use of CVs grows
much, the existing restriction is going to become untenable anyway,
so why not just get rid of it?

regards, tom lane

Re: PATCH: pgbench - option to build using ppoll() for larger connection counts

[Stephen Frost]

Doug,

* Rady, Doug (radyd...@amazon.com) wrote:
> This patch enables building pgbench to use ppoll() instead of select()
> to allow for more than (FD_SETSIZE - 10) connections.  As implemented,
> when using ppoll(), the only connection limitation is system resources.

Looks like this patch had some good feedback back when it was posted,
but we haven't seen any update to it based on that feedback and we're
now nearly a week into the January commitfest.

In order to keep the commitfest moving and to give you the best
opportunity at having the patch be reviewed and committed during this
cycle, I'd suggest trying to find some time soon to incorporate the
recommendations provided and post an updated patch for review.

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: General purpose hashing func in pgbench

[Stephen Frost]

Greetings Ildar,

* Fabien COELHO (coe...@cri.ensmp.fr) wrote:
> >>I noticed from the source of all human knowledege (aka Wikipedia:-)
> >>that there seems to be a murmur3 successor. Have you considered it?
> >>One good reason to skip it would be that the implementation is long
> >>and complex. I'm not sure about a 8-byte input simplified version.
> >Murmur2 naturally supports 8-byte data. Murmur3 has 32- and 128-bit
> >versions.
> 
> Ok.
> 
> So it would make sense to keep the 64 bit murmur2 version.
> 
> Pgbench ints are 64 bits, seems logical to keep them that way, so 32 bits
> versions do not look too interesting.

This sounds like it was settled and the patch needs updating with these
changes, and with documentation and tests added.

While the commitfest is still pretty early on, I would suggest trying to
find time soon to update the patch and resubmit it, so it can get
another review and hopefully be included during this commitfest.

Also, I've not checked myself, but the patch appears to be failing for
http://commitfest.cputube.org/ so it likely also needs a rebase.

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: [HACKERS] GUC for cleanup indexes threshold.

[Stephen Frost]

Greetings,

(pruned the CC list)

* Michael Paquier (michael.paqu...@gmail.com) wrote:
> On Thu, Nov 30, 2017 at 12:06 AM, Masahiko Sawada  
> wrote:
> > On Wed, Nov 29, 2017 at 11:05 PM, Simon Riggs  wrote:
> >> Unless there is disagreement on the above, it seems we should apply
> >> Yura's patch (an edited version, perhaps).
> >>
> >
> > IIRC the patches that makes the cleanup scan skip has a problem
> > pointed by Peter[1], that is that we stash an XID when a btree page is
> > deleted, which is used to determine when it's finally safe to recycle
> > the page. Yura's patch doesn't have that problem?
> >
> > [1] 
> > https://www.postgresql.org/message-id/CAH2-Wz%3D1%3Dt5fcGGfarQGcAWBqaCh%2BdLMjpYCYHpEyzK8Qg6OrQ%40mail.gmail.com
> 
> The latest patch present on this thread does not apply
> (https://www.postgresql.org/message-id/c4a47caef28024ab7528946bed264...@postgrespro.ru).
> Please send a rebase. For now I am moving the patch to next CF, with
> "waiting on author".

We are now just about a week into the CF and this patch hasn't been
updated and is still in 'waiting on author' state and it sounds like it
might have a pretty serious issue based on the above comment.

Masahiko Sawada, if this patch isn't viable or requires serious rework
to be acceptable, then perhaps we should change its status to 'returned
with feedback' and you can post a new patch for the next commitfest..?

Otherwise, I'd encourage you to try and find time to post an updated
patch soon, so that it can be reviewed.

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: [HACKERS] [PATCH] Vacuum: Update FSM more frequently

[Stephen Frost]

Greetings Claudio,

* Michael Paquier (michael.paqu...@gmail.com) wrote:
> On Mon, Nov 27, 2017 at 2:39 PM, Jing Wang  wrote:
> > A few general comments.
> >
> > +   FreeSpaceMapVacuum(onerel, 64);
> >
> > Just want to know why '64' is used here? It's better to give a description.
> >
> > +else
> > +   {
> > +   newslot = fsm_get_avail(page, 0);
> > +   }
> >
> > Since there is only one line in the else the bracket will not be needed. And
> > there in one more space ahead of else which should be removed.
> >
> >
> > +   if (nindexes == 0 &&
> > +   (vacuumed_pages_at_fsm_vac - vacuumed_pages) >
> > vacuum_fsm_every_pages)
> > +   {
> > +   /* Vacuum the Free Space Map */
> > +   FreeSpaceMapVacuum(onerel, 0);
> > +   vacuumed_pages_at_fsm_vac = vacuumed_pages;
> > +   }
> >
> > vacuumed_pages_at_fsm_vac is initialised with value zero and seems no chance
> > to go into the bracket.
> 
> The patch presented still applies, and there has been a review two
> days ago. This is a short delay to answer for the author, so I am
> moving this patch to next CF with the same status of waiting on
> author.

Looks like this patch probably still applies and the changes suggested
above seem pretty small, any chance you can provide an updated patch
soon Claudio, so that it can be reviewed properly during this
commitfest?

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: [HACKERS] Vacuum: allow usage of more than 1GB of work mem

[Stephen Frost]

Greetings,

* Michael Paquier (michael.paqu...@gmail.com) wrote:
> On Mon, Dec 4, 2017 at 2:38 PM, Claudio Freire  wrote:
> > They did apply at the time, but I think major work on vacuum was
> > pushed since then, and also I was traveling so out of reach.
> >
> > It may take some time to rebase them again. Should I move to needs
> > review myself after that?
> 
> Sure, if you can get into this state, please feel free to update the
> status of the patch yourself.

We're now over a month since this status update- Claudio, for this to
have a chance during this commitfest to be included (which, personally,
I think would be great as it solves a pretty serious issue..), we really
need to have it be rebased and updated.  Once that's done, as Michael
says, please change the patch status back to 'Needs Review'.

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: [HACKERS] Cached plans and statement generalization

[Stephen Frost]

Greetings,

* Konstantin Knizhnik (k.knizh...@postgrespro.ru) wrote:
> On 30.11.2017 04:59, Michael Paquier wrote:
> >On Wed, Sep 13, 2017 at 2:11 AM, Konstantin Knizhnik
> > wrote:
> >>One more patch passing all regression tests with autoprepare_threshold=1.
> >>I still do not think that it should be switch on by default...
> >This patch does not apply, and did not get any reviews. So I am moving
> >it to next CF with waiting on author as status. Please provide a
> >rebased version. Tsunakawa-san, you are listed as a reviewer of this
> >patch. If you are not planning to look at it anymore, you may want to
> >remove your name from the related CF entry
> >https://commitfest.postgresql.org/16/1150/.
> 
> Updated version of the patch is attached.

This patch appears to apply with just a bit of fuzz and make check
passes, so I'm not sure why this is currently marked as 'Waiting for
author'.

I've updated it to be 'Needs review'.  If that's incorrect, feel free to
change it back with an explanation.

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: [HACKERS] SQL/JSON in PostgreSQL

[Nikita Glukhov]

On 07.01.2018 00:11, Pavel Stehule wrote:

2018-01-06 22:02 GMT+01:00 Oleg Bartunov >:


On Sat, Jan 6, 2018 at 8:22 AM, Pavel Stehule
mailto:pavel.steh...@gmail.com>> wrote:
> Hi
>
> I am checking the JSONPath related code
>
> Questions, notes:
>
> 1. jsonpath operators are not consistent with any other .. json,
xml .. I am
> missing ?, @> operátors

I have slides about jsonpath
http://www.sai.msu.su/~megera/postgres/talks/sqljson-pgconf.eu-2017.pdf


> 2. documentation issue - there is "'{"a":[1,2,3,4,5]}'::json *?
'$.a[*] ? (@
>> 2)'" - operator *? doesn't exists

There are should be @? operator

> 3. operator @~ looks like too aggressive shortcut - should be better
> commented
>
> What is not clean, if jsonpath should to create some new
operators for json,
> jsonb types? It is special filter, defined by type, so from my
perspective
> the special operators are not necessary.

It's impossible to distinguish jsonpath from text, so introducing
new operators
are easier than everytime explicitly specify jsonpath datatype.


There are two possible solutions - special operator or explicit 
casting. In this case I am not sure if special operator for this case 
is good solution. Probably nobody will use it - because there SQL/JSON 
functions, but I don't think so this inconsistency is correct.


I have not strong opinion about it - it will be hidden feature for 
almost all users.



Operators are necessary for index support now.

Operators allows us to use a more concise syntax in simple cases, when 
we extract JSON item(s) without error handling:

js @* '$.key'
vs
JSON_QUERY(js, '$.key' RETURNING jsonb ERROR ON ERROR)


Also @* оperator gives us ability to extract a set of JSON items. 
JSON_QUERY can only wrap extracted item sequence into JSON array which 
we have to unwrap with our json[b]_array_elements() function. I also 
thought about returning setof-types in JSON_VALUE/JSON_QUERY:


JSON_QUERY(jsonb '[1,2,3]', '$[*]' RETURNING SETOF jsonb)

But it is not so easy to implement now, because we should introduce new 
node like TableFunc (or also we can try to use existing JSON_TABLE 
infrastructure).


Set-returning expressions are not allowed in every context, so for 
returning singleton items there should be additional operator.


--
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: [HACKERS] Surjective functional indexes

[Stephen Frost]

Greetings,

* Konstantin Knizhnik (k.knizh...@postgrespro.ru) wrote:
> On 15.12.2017 01:21, Michael Paquier wrote:
> >On Fri, Dec 15, 2017 at 6:15 AM, Alvaro Herrera  
> >wrote:
> >>Konstantin Knizhnik wrote:
> >>>If you still thing that additional 16 bytes per relation in statistic is 
> >>>too
> >>>high overhead, then I will also remove autotune.
> >>I think it's pretty clear that these additional bytes are excessive.
> >The bar to add new fields in PgStat_TableCounts in very high, and one
> >attempt to tackle its scaling problems with many relations is here by
> >Horiguchi-san:
> >https://www.postgresql.org/message-id/20171211.201523.24172046.horiguchi.kyot...@lab.ntt.co.jp
> >His patch may be worth a look if you need more fields for your
> >feature. So it seems to me that the patch as currently presented has
> >close to zero chance to be committed as long as you keep your changes
> >to pgstat.c.
> 
> Ok, looks like everybody think that autotune based on statistic is bad idea.
> Attached please find patch without autotune.

This patch appears to apply with a reasonable amount of fuzz, builds,
and passes 'make check', at least, therefore I'm going to mark it
'Needs Review'.

I will note that the documentation doesn't currently build due to this:

/home/sfrost/git/pg/dev/cleanup/doc/src/sgml/ref/create_index.sgml:302: parser 
error : Opening and ending tag mismatch: literal line 302 and unparseable
recheck_on_update

but I don't think it makes sense for that to stand in the way of someone
doing a review of the base patch.  Still, please do fix the
documentation build when you get a chance.

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: [HACKERS] Secondary index access optimizations

[Stephen Frost]

Greetings,

* Konstantin Knizhnik (k.knizh...@postgrespro.ru) wrote:
> On 04.12.2017 19:44, Alvaro Herrera wrote:
> >Konstantin Knizhnik wrote:
> >>
> >>On 30.11.2017 05:16, Michael Paquier wrote:
> >>>On Mon, Nov 6, 2017 at 10:13 PM, Konstantin Knizhnik
> >>> wrote:
> Concerning broken partition_join test: it is "expected" failure: my patch
> removes from the plans redundant checks.
> So the only required action is to update expected file with results.
> Attached please find updated patch.
> >>>The last patch version has conflicts in regression tests and did not
> >>>get any reviews. Please provide a rebased version. I am moving the
> >>>patch to next CF with waiting on author as status. Thanks!
> >>Rebased patch is attached.
> >I don't think this is a rebase on the previously posted patch ... it's
> >about 10x as big and appears to be a thorough rewrite of the entire
> >optimizer.
> 
> Or, sorry. I really occasionally committed in this branch patch for
> aggregate push down.
> Correct reabased patch is attached.

This patch applies, builds and passes 'make check-world', with no real
review posted of it, so I don't believe it should be 'Waiting on Author'
but really in 'Needs Review' status, so I've gone ahead and updated the
CF with that status.

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: [HACKERS] [PATCH] Overestimated filter cost and its mitigation

[Stephen Frost]

Greetings,

* Michael Paquier (michael.paqu...@gmail.com) wrote:
> On Thu, Nov 9, 2017 at 12:33 PM, Ashutosh Bapat
>  wrote:
> > Looking at order_qual_clauses(), we can say that a set of quals q1
> >  qn are ordered the same irrespective of the set of clauses they
> > are subset of. E.g. if {q1 .. qn} is subset of Q (ordered as Qo) and
> > also Q' (ordered as Q'o) the order in which they appear in Qo and Q'o
> > is same. So, even if different paths segregate the clauses in
> > different set, within each set the order is same. FWIW, we can order
> > all clauses in largest set once and use that order every time. Albeit
> > we will have to remember the order somewhere OR make the separator
> > routine retain the order in the larger set, which I guess is true
> > about all separator functions.
> 
> I am not sure what to think about this patch, so moved to next CF. The
> patch still applies. Hayamizu-san, it would be nice as well if you
> could review other's patches. One patch reviewed for one patch
> submitted, with equal difficulty. You should also get a community
> account so as it is possible to add your name as an author of this
> patch.

While this patch does still apply, it doesn't pass the 'make check'
regression tests and there appears to be concern raised up-thread about
the performance impact.

Yuto Hayamizu, it looks like some good questions were raised and which
need to be addressed, in addition to making sure that the patch at least
passes 'make check', so I'm leaving this as waiting-for-author.  If you
believe the patch is no longer viable, we can change it to 'returned
with feedback', otherwise, an updated patch and some responses to the
questions raised would be great as we're already a week into this
commitfest and this thread has been dormant since near the start of the
last CF.

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: [HACKERS] GUC for cleanup indexes threshold.

[Peter Geoghegan]

On Sat, Jan 6, 2018 at 2:20 PM, Stephen Frost  wrote:
>> > IIRC the patches that makes the cleanup scan skip has a problem
>> > pointed by Peter[1], that is that we stash an XID when a btree page is
>> > deleted, which is used to determine when it's finally safe to recycle
>> > the page. Yura's patch doesn't have that problem?
>> >
>> > [1] 
>> > https://www.postgresql.org/message-id/CAH2-Wz%3D1%3Dt5fcGGfarQGcAWBqaCh%2BdLMjpYCYHpEyzK8Qg6OrQ%40mail.gmail.com

> Masahiko Sawada, if this patch isn't viable or requires serious rework
> to be acceptable, then perhaps we should change its status to 'returned
> with feedback' and you can post a new patch for the next commitfest..?

I believe that the problem that I pointed out with freezing/wraparound
is a solvable problem. If we think about it carefully, we will come up
with a good solution. I have tried to get the ball rolling with my
pd_prune_xid suggestion. I think it's important to not lose sight of
the fact that the page deletion/recycling XID thing is just one detail
that we need to think about some more.

I cannot fault Sawada-san for waiting to hear other people's views
before proceeding. It really needs to be properly discussed.

-- 
Peter Geoghegan

Re: to_typemod(type_name) information function

[Tom Lane]

Sophie Herold  writes:
> the following patch allows to retrieve the typemod. Without this patch,
> it does not seem to be possible to generate the first column.

I thought about this a bit, and I now follow the problem you want to
solve, and agree that format_type() is going in the wrong direction.
However, the proposed solution seems a bit grotty.  You're basically
always going to need to run parseTypeString twice on the same input,
because there are few if any use-cases for getting just the typmod
without the type OID.  I think it might be more useful to provide
a single function

parse_type(type_name text, OUT typeid regtype, OUT typmod integer)

which would replace both to_regtype and to_typemod in your example.
Usage might look like

SELECT format_type(typeid, typmod)
  FROM (VALUES
('INTERVAL SECOND (5)'),
('Varchar(17)'),
('timestamptz (2)')) AS x(t), parse_type(x.t);

Creating a function with multiple OUT parameters at the pg_proc.h level
is slightly painful, but see e.g. pg_sequence_parameters for a model.

regards, tom lane

Re: [HACKERS] plpgsql - additional extra checks

[Stephen Frost]

Greetings Pavel,

* Pavel Stehule (pavel.steh...@gmail.com) wrote:
> 2017-11-30 3:44 GMT+01:00 Michael Paquier :
> > At least documentation needs patching, or this is going to generate
> > warnings on HEAD at compilation. I am moving this to next CF for lack
> > of reviews, and the status is waiting on author as this needs at least
> > a couple of doc fixes.
> 
> fixed doc attached

Looks like this patch should have been in "Needs Review" state, not
"Waiting for author", since it does apply, build and pass make
check-world, but as I'm signed up to review it, I'll do so here:

diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml
index 7d23ed437e..efa48bc13c 100644
--- a/doc/src/sgml/plpgsql.sgml
+++ b/doc/src/sgml/plpgsql.sgml
@@ -4963,6 +4963,11 @@ a_output := a_output || $$ if v_$$ || referrer_keys.kind 
|| $$ like '$$
 so you are advised to test in a separate development environment.

 
+   
+The setting plpgsql.extra_warnings to 
all is a 
+good idea in developer or test environments.
+   

Better language for this would be:

Setting plpgsql.extra_warnings, or
plpgsql.extra_errors, as appropriate, to
all is encouraged in development and/or testing
environments.

@@ -4979,6 +4984,30 @@ a_output := a_output || $$ if v_$$ || referrer_keys.kind 
|| $$ like '$$
  
 

+
+   
+strict_multi_assignment
+
+ 
+  Some PL/PgSQL commands allows to assign a 
values to
+  more than one variable. The number of target variables should not be
+  equal to number of source values. Missing values are replaced by NULL
+  value, spare values are ignored. More times this situation signalize
+  some error.
+ 
+
+   

Better language:

Some PL/PgSQL commands allow assigning values
to more than one variable at a time, such as SELECT INTO.  Typically,
the number of target variables and the number of source variables should
match, though PL/PgSQL will use NULL for
missing values and extra variables are ignored.  Enabling this check
will cause PL/PgSQL to throw a WARNING or
ERROR whenever the number of target variables and the number of source
variables are different.

+   
+too_many_rows
+
+ 
+  When result is assigned to a variable by INTO clause,
+  checks if query returns more than one row. In this case the assignment
+  is not deterministic usually - and it can be signal some issues in 
design.
+ 
+
+   
   

Better language:

Enabling this check will cause PL/PgSQL to
check if a given query returns more than one row when an
INTO clause is used.  As an INTO statement will only
ever use one row, having a query return multiple rows is generally
either inefficient and/or nondeterministic and therefore is likely an
error.

@@ -4997,6 +5026,34 @@ WARNING:  variable "f1" shadows a previously defined 
variable
 LINE 3: f1 int;
 ^
 CREATE FUNCTION
+
+
+  The another example shows the effect of 
plpgsql.extra_warnings
+  set to strict_multi_assignment:
+

Better language:

The below example shows the effects of setting
plpgsql.extra_warnings to
strict_multi_assignment:

diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index ec480cb0ba..0879e84cd2 100644
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -3629,6 +3629,24 @@ exec_stmt_execsql(PLpgSQL_execstate *estate,
longtcount;
int rc;
PLpgSQL_expr *expr = stmt->sqlstmt;
+   booltoo_many_rows_check;
+   int too_many_rows_level;
+
+   if (plpgsql_extra_errors & PLPGSQL_XCHECK_TOOMANYROWS)
+   {
+   too_many_rows_check = true;
+   too_many_rows_level = ERROR;
+   }
+   else if (plpgsql_extra_warnings & PLPGSQL_XCHECK_TOOMANYROWS)
+   {
+   too_many_rows_check = true;
+   too_many_rows_level = WARNING;
+   }
+   else
+   {
+   too_many_rows_check = false;
+   too_many_rows_level = NOTICE;
+   }


I'm not sure why we need two variables here- couldn't we simply look at
too_many_rows_level?  eg: too_many_rows_level >= WARNING ? ...

Not as big a deal, but I would change it to be 'check_too_many_rows' as
a variable name too.
 
@@ -3678,7 +3696,7 @@ exec_stmt_execsql(PLpgSQL_execstate *estate,
 */
if (stmt->into)
{
-   if (stmt->strict || stmt->mod_stmt)
+   if (stmt->strict || stmt->mod_stmt || too_many_rows_check)
tcount = 2;
else
tcount = 1;

The comment above this block needs updating for this change and, in
general, there's probably other pieces of code that this patch
changes where the comments should either be improved or ones added.


@@ -6033,12 +6051,48 @@ exec_move_row(PLpgSQL_execstate *estate,
int t_natts;
int fnum;
int 

Re: [HACKERS] GUC for cleanup indexes threshold.

[Stephen Frost]

Greetings Peter,

* Peter Geoghegan (p...@bowt.ie) wrote:
> On Sat, Jan 6, 2018 at 2:20 PM, Stephen Frost  wrote:
> >> > IIRC the patches that makes the cleanup scan skip has a problem
> >> > pointed by Peter[1], that is that we stash an XID when a btree page is
> >> > deleted, which is used to determine when it's finally safe to recycle
> >> > the page. Yura's patch doesn't have that problem?
> >> >
> >> > [1] 
> >> > https://www.postgresql.org/message-id/CAH2-Wz%3D1%3Dt5fcGGfarQGcAWBqaCh%2BdLMjpYCYHpEyzK8Qg6OrQ%40mail.gmail.com
> 
> > Masahiko Sawada, if this patch isn't viable or requires serious rework
> > to be acceptable, then perhaps we should change its status to 'returned
> > with feedback' and you can post a new patch for the next commitfest..?
> 
> I believe that the problem that I pointed out with freezing/wraparound
> is a solvable problem. If we think about it carefully, we will come up
> with a good solution. I have tried to get the ball rolling with my
> pd_prune_xid suggestion. I think it's important to not lose sight of
> the fact that the page deletion/recycling XID thing is just one detail
> that we need to think about some more.
> 
> I cannot fault Sawada-san for waiting to hear other people's views
> before proceeding. It really needs to be properly discussed.

Thanks for commenting!

Perhaps it should really be in Needs review state then..?

Either way, thanks again for the clarification and hopefully this will
revive the discussion.

Stephen


signature.asc
Description: PGP signature

Parallel append plan instability/randomness

[Tom Lane]

According to buildfarm member silverfish,

https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=silverfish&dt=2018-01-06%2008%3A53%3A38

it's possible to sometimes get this failure in the regression tests:

*** 
/mnt/buildfarm/buildroot/HEAD/pgsql.build/../pgsql/src/test/regress/expected/select_parallel.out
Tue Dec 19 20:24:02 2017
--- 
/mnt/buildfarm/buildroot/HEAD/pgsql.build/src/test/regress/results/select_parallel.out
  Sat Jan  6 09:21:39 2018
***
*** 75,84 
   Workers Planned: 3
   ->  Partial Aggregate
 ->  Parallel Append
   ->  Seq Scan on d_star
   ->  Seq Scan on f_star
   ->  Seq Scan on e_star
-  ->  Seq Scan on b_star
   ->  Seq Scan on c_star
   ->  Seq Scan on a_star
  (11 rows)
--- 75,84 
   Workers Planned: 3
   ->  Partial Aggregate
 ->  Parallel Append
+  ->  Seq Scan on b_star
   ->  Seq Scan on d_star
   ->  Seq Scan on f_star
   ->  Seq Scan on e_star
   ->  Seq Scan on c_star
   ->  Seq Scan on a_star
  (11 rows)

Irreproducible failures in the regression tests are not very acceptable.
Furthermore, considering that the query being tested is

explain (costs off)
  select round(avg(aa)), sum(aa) from a_star;

it seems to me that the "expected" order of the sub-scans is mighty
random to begin with.  A non-parallel implementation of the same
query will consistently scan these tables in their inheritance order:

 Aggregate
   ->  Append
 ->  Seq Scan on a_star
 ->  Seq Scan on b_star
 ->  Seq Scan on c_star
 ->  Seq Scan on d_star
 ->  Seq Scan on e_star
 ->  Seq Scan on f_star

Could we fix the parallel case to behave similarly please?

regards, tom lane

Re: [HACKERS] Refactoring identifier checks to consistently use strcmp

[Stephen Frost]

Greetings Michael, Daniel, all,

* Michael Paquier (michael.paqu...@gmail.com) wrote:
> On Fri, Dec 1, 2017 at 4:14 AM, Robert Haas  wrote:
> > I think the changes in DefineView and ATExecSetRelOptions is wrong,
> > because transformRelOptions() is still using pg_strcasecmp.  With the
> > patch:
> >
> > rhaas=# create view v(x) with ("Check_option"="local") as select 1;
> > CREATE VIEW
> > rhaas=# create view v(x) with ("check_option"="local") as select 1;
> > ERROR:  WITH CHECK OPTION is supported only on automatically updatable views
> > HINT:  Views that do not select from a single table or view are not
> > automatically updatable.
> >
> > Oops.
> 
> I am getting the feeling that there could be other issues than this
> one... If this patch ever gets integrated, I think that this should
> actually be shaped as two patches:
> - One patch with the set of DDL queries taking advantage of the
> current grammar with pg_strcasecmp.
> - A second patch that does the switch from pg_strcasecmp to strcmp,
> and allows checking which paths of patch 1 are getting changed.
> Patch 1 is the hard part to figure out all the possible patterns that
> could be changed.
> 
> > I am actually pretty dubious that we want to do this.  I found one bug
> > already (see above), and I don't think there's any guarantee that it's
> > the only one, because it's pretty hard to be sure that none of the
> > remaining calls to pg_strcasecmp are being applied to any of these
> > values in some other part of the code.  I'm not sure that the backward
> > compatibility issue is a huge deal, but from my point of view this
> > carries a significant risk of introducing new bugs, might annoy users
> > who spell any of these keywords in all caps with surrounding quotation
> > marks, and really has no clear benefit that I can see.  The
> > originally-offered justification is that making this consistent would
> > help us avoid subtle bugs, but it seems at least as likely to CREATE
> > subtle bugs, and the status quo is AFAICT harming nobody.
> 
> Changing opinion here ;)
> Yes, I agree that the risk of bugs may not be worth the compatibility
> effort at the end. I still see value in this patch for long-term
> purposes by making the code more consistent though.

Looks like this discussion has progressed to where this patch should
really be marked as Rejected.  Does anyone else want to voice an opinion
regarding it, or perhaps Daniel could post a rebuttal to the concerns
raised here?

Thinking through it, for my own 2c on this, I tend to agree with Tom
that, really, we should be using strcmp() for anything coming out of the
parser and that the backward-compatibility break from that is
acceptable.  I also understand Robert's concern that there may be other
bugs hiding and I wonder if there might be a way to check for them,
though no great ideas spring to mind offhand.  Would be great to hear
your thoughts, Daniel, so leaving this in Waiting on Author for now.

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: [HACKERS] Refactoring identifier checks to consistently use strcmp

[Tom Lane]

Stephen Frost  writes:
> Thinking through it, for my own 2c on this, I tend to agree with Tom
> that, really, we should be using strcmp() for anything coming out of the
> parser and that the backward-compatibility break from that is
> acceptable.  I also understand Robert's concern that there may be other
> bugs hiding and I wonder if there might be a way to check for them,
> though no great ideas spring to mind offhand.  Would be great to hear
> your thoughts, Daniel, so leaving this in Waiting on Author for now.

FWIW, I don't especially agree with Robert's position, because I think
he is ignoring the argument that it's a bug that some things are
case-sensitive and other seemingly similar things are not.

It's definitely concerning that the submitted patch introduced a new bug,
but we have seldom taken the position that bugs in an initial submission
are sufficient grounds for rejecting the entire concept.

ISTM that if this patch gets rid of a large fraction of the uses of
pg_strcasecmp in the backend, which is what I expect it should, then
it might not be out of reach to go through all the surviving ones to
make sure they are not processing strings that originate in the parser.

regards, tom lane

Re: proposal: alternative psql commands quit and exit

[Chapman Flack]

I'm not sure what the usual shape of 'consensus' or 'resolution' is in these 
things,
but it seems to me that the branch of this email thread that leads here 
(through the
message from Robert that I'm replying to) is the one that felt like it was 
converging.

I thought it was converging on the idea that

  ^[whitespace]*(quit|exit)[whitespace]*(;[whitespace]*)?$

would be treated specially when interactive, whether on a first or a 
continuation line,
and that the special treatment would be a helpful explanatory message to the
interactive user, while nevertheless appending the text to the query buffer.

One possibly not-yet-converged point was whether the special treatment should
be the same on a first line, or should just actually quit in that case.

None of this is what the currently latest patch does, though.

-Chap

Re: [HACKERS] GUC for cleanup indexes threshold.

[Peter Geoghegan]

Hi Stephen,

On Sat, Jan 6, 2018 at 4:02 PM, Stephen Frost  wrote:
> Perhaps it should really be in Needs review state then..?

Probably.

As I pointed out already some time ago, this RecentGlobalXmin
interlock stuff is our particular implementation of what Lanin &
Shasha call "The Drain Technique" within "2.5 Freeing Empty Nodes".
It's not easy to understand why this is necessary in general (you have
to understand the whole paper for that), but our implementation of the
drain technique is simple. I'm afraid that I made the problem sound
scarier than it actually is.

As Lanin & Shasha put it: "freeing empty nodes by the drain technique
is transparent to the rest of the algorithm and can be added by a
separate module". nbtree doesn't actually care very much about XIDs --
testing an XID against RecentGlobalXmin was just the most convenient
way of using L&S's technique to defer recycling until it is definitely
safe. We only need to make sure that _bt_page_recyclable() cannot
become confused by XID wraparound to fix this problem -- that's it.

-- 
Peter Geoghegan

Re: proposal: alternative psql commands quit and exit

[Tom Lane]

Chapman Flack  writes:
> I'm not sure what the usual shape of 'consensus' or 'resolution' is in these 
> things,
> but it seems to me that the branch of this email thread that leads here 
> (through the
> message from Robert that I'm replying to) is the one that felt like it was 
> converging.

> I thought it was converging on the idea that

>   ^[whitespace]*(quit|exit)[whitespace]*(;[whitespace]*)?$

> would be treated specially when interactive, whether on a first or a 
> continuation line,
> and that the special treatment would be a helpful explanatory message to the
> interactive user, while nevertheless appending the text to the query buffer.

That's what I thought, too.  I also think that we should recognize "help"
under exactly the same circumstances --- right now only a subset of what
you specify here will trigger the help response.

> One possibly not-yet-converged point was whether the special treatment should
> be the same on a first line, or should just actually quit in that case.

I can get on board with actually quitting if it's on the first line,
which is equivalent to the current behavior for "help".  That is,
all three would do-what-it-says-on-the-tin if entered when the query
buffer is empty, while they'd all emit an explanatory message (maybe
the same for all three, or maybe not) if entered when the query
buffer is not empty.

I doubt we have consensus on just what the explanatory message should
say, but maybe the author can give us a proposal to shoot at.

regards, tom lane

Re: Fix a Oracle-compatible instr function in the documentation

[Tom Lane]

Tatsuo Ishii  writes:
>> The documentation of PL/pgSQL provides sample codes of Oracle-compatible
>> instr functions. However, the behaviour is a little differet.
>> Oracle's instr raises an error when the forth argument value is less than
>> zero, but the sample code returns zero. This patch fixes this.

> Your patch fixes only instr(string varchar, string_to_search varchar, 
> beg_index integer).
> However in the doc there is another instr(string varchar, string_to_search 
> varchar, beg_index integer, occur_index integer).

> Shouldn't this be fixed as well?

Nagata-san's proposed addition makes no sense in the second instr()
implementation, which has no occur_index parameter; it only makes sense
in the third one.  I think the submitted patch is probably against some
old version of plpgsql.sgml and the line numbers in it are confusing
"patch" into thinking it should go into the second instr() implementation.

I checked with http://rextester.com/l/oracle_online_compiler
and verified that Oracle throws an error for zero/negative occur_index,
so the patch is definitely correct as far as it goes.  However, poking
at it a bit further, there is another problem: our sample code disagrees
with Oracle about what negative beg_index means.  For example, our code
produces

regression=# select instr('foo bar baz bar quux', 'bar', -6) ;
 instr 
---
13
(1 row)

regression=# select instr('foo bar baz bar quux', 'bar', -7) ;
 instr 
---
 5
(1 row)

whereas I get this with Oracle:

select instr('foo bar baz bar quux', 'bar', -6) from dual
13
select instr('foo bar baz bar quux', 'bar', -7) from dual
13
select instr('foo bar baz bar quux', 'bar', -8) from dual
13
select instr('foo bar baz bar quux', 'bar', -9) from dual
5

Evidently, they consider that negative beg_index indicates
the last place where the target substring can *begin*, whereas
our code thinks it is the last place where the target can *end*.

After a bit of fooling around with it, I produced code that agrees
with Oracle as far as I can tell, but it wouldn't be a bad idea
for someone else to test it some more.

I eliminated a couple of obvious ineffiencies while at it, notably
using position() for what could be a simple equality test in the
backwards-search loops.  I also changed the header comment, which
was both very incomplete and wrong in detail.

While the response to negative occur_index is probably not that
interesting in the field, the non-equivalent behavior for negative
beg_index is very definitely something that could bite people doing
Oracle translations.  So I agree we'd better back-patch this, and
maybe even call it out as a bug fix in the release notes.

regards, tom lane

diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml
index 7d23ed4..9d9e362 100644
*** a/doc/src/sgml/plpgsql.sgml
--- b/doc/src/sgml/plpgsql.sgml
*** $$ LANGUAGE plpgsql STRICT IMMUTABLE;
*** 5650,5668 
  
  --
  -- instr functions that mimic Oracle's counterpart
! -- Syntax: instr(string1, string2, [n], [m]) where [] denotes optional parameters.
  --
! -- Searches string1 beginning at the nth character for the mth occurrence
! -- of string2.  If n is negative, search backwards.  If m is not passed,
! -- assume 1 (search starts at first character).
  --
  
  CREATE FUNCTION instr(varchar, varchar) RETURNS integer AS $$
- DECLARE
- pos integer;
  BEGIN
! pos:= instr($1, $2, 1);
! RETURN pos;
  END;
  $$ LANGUAGE plpgsql STRICT IMMUTABLE;
  
--- 5650,5668 
  
  --
  -- instr functions that mimic Oracle's counterpart
! -- Syntax: instr(string1, string2 [, n [, m]]) where [] denotes optional parameters.
  --
! -- Search string1, beginning at the nth character, for the mth occurrence
! -- of string2.  If n is negative, search backwards, starting at the abs(n)'th
! -- character from the end of string1.
! -- If n is not passed, assume 1 (search starts at first character).
! -- If m is not passed, assume 1 (find first occurrence).
! -- Returns starting index of string2 in string1, or 0 if string2 is not found.
  --
  
  CREATE FUNCTION instr(varchar, varchar) RETURNS integer AS $$
  BEGIN
! RETURN instr($1, $2, 1);
  END;
  $$ LANGUAGE plpgsql STRICT IMMUTABLE;
  
*** BEGIN
*** 5688,5700 
  ELSIF beg_index < 0 THEN
  ss_length := char_length(string_to_search);
  length := char_length(string);
! beg := length + beg_index - ss_length + 2;
  
  WHILE beg > 0 LOOP
  temp_str := substring(string FROM beg FOR ss_length);
! pos := position(string_to_search IN temp_str);
! 
! IF pos > 0 THEN
  RETURN beg;
  END IF;
  
--- 5688,5698 
  ELSIF beg_index < 0 THEN
  ss_length := char_length(string_to_search);
  length := char_length(string);
! beg := length + 1 + beg_index;
  
  WHILE beg > 0 LOOP
  temp_str := substring(string FROM beg FOR s

Re: Add default role 'pg_access_server_files'

[Ryan Murphy]

Hi Stephen,

I have another question then based on what you said earlier today, and some 
testing I did using your patch.

TLDR: I created a role "tester" and was (as expected) not able to perform 
pg_read_file() on files outside the data directory.
But then I granted EXECUTE on that function for that role, and then I was able 
to (which is not what I expected).

Here's what I did (I apologize if this is too verbose):

* I successfully applied your patch to HEAD, and built Postgres from source:

make clean
configure (with options including a specific --prefix)
make
make install

* Then I went into the "install_dir/bin" and did the following to setup a data 
directory:

$ ./initdb ~/sql-data/2018-01-06
The files belonging to this database system will be owned by user 
"postgres".
This user must also own the server process.

The database cluster will be initialized with locale "en_US.UTF-8".
The default database encoding has accordingly been set to "UTF8".
The default text search configuration will be set to "english".

Data page checksums are disabled.

creating directory /Users/postgres/sql-data/2018-01-06 ... ok
creating subdirectories ... ok
selecting default max_connections ... 100
selecting default shared_buffers ... 128MB
selecting dynamic shared memory implementation ... posix
creating configuration files ... ok
running bootstrap script ... ok
performing post-bootstrap initialization ... ok
syncing data to disk ... ok

WARNING: enabling "trust" authentication for local connections
You can change this by editing pg_hba.conf or using the option -A, or
--auth-local and --auth-host, the next time you run initdb.

Success. You can now start the database server using:

./pg_ctl -D /Users/postgres/sql-data/2018-01-06 -l logfile start

* Then I started the database:

$ ./pg_ctl -D /Users/postgres/sql-data/2018-01-06 -l logfile start
waiting for server to start done
server started

* I went into the database and tried a pg_read_file:

$ psql postgres
psql (9.4.5, server 11devel)
Type "help" for help.

postgres=# select pg_read_file('/Users/postgres/temp');
   pg_read_file
---
 here is the file content
 
(1 row)

* Of course that worked as superuser, so created a new role:

postgres=# create role tester;
CREATE ROLE
postgres=# \q
postgres=# alter role tester with login;
ALTER ROLE
postgres=# \q

$ psql postgres tester
psql (9.4.5, server 11devel)
Type "help" for help.

postgres=> select pg_read_file('/Users/postgres/temp');
ERROR:  permission denied for function pg_read_file
postgres=> \q

* My current understanding at this point is that EXECUTE permissions would only 
allow "tester" to pg_read_file() on files in the data directory.  I try 
GRANTing EXECUTE:

$ psql postgres
psql (9.4.5, server 11devel)
Type "help" for help.

postgres=# grant execute on function pg_read_file(text) to tester;
GRANT
postgres=# select pg_read_file('/Users/postgres/temp');
   pg_read_file
---
 here is the file content
 
(1 row)



Is this expected behavior?  I thought I would need to GRANT that new 
"pg_access_server_files" role to "tester" in order to do this.  I may have 
misunderstood how your new feature works, just doublechecking.

Regards,
Ryan

Re: Add default role 'pg_access_server_files'

[Ryan Murphy]

Oops!  I made a mistake, which clearly showed up in my last email: I forgot
to psql back in as "tester".

Now I get the right behavior:

$ psql postgres tester
psql (9.4.5, server 11devel)
Type "help" for help.

postgres=> select pg_read_file('/Users/postgres/temp');
ERROR:  absolute path not allowed

Thanks for bearing with me.  So far so good on this feature, going to run
the tests too.

Best,
Ryan

Re: Add default role 'pg_access_server_files'

[Ryan Murphy]

(Duplicated to make sure it's in the commitfest Thread, didn't seem to get in 
there when I replied to the email)

Oops!  I made a mistake, which clearly showed up in my last email: I forgot to 
psql back in as "tester".

Now I get the right behavior:

$ psql postgres tester
postgres=> select pg_read_file('/Users/postgres/temp');
ERROR:  absolute path not allowed

Thanks for bearing with me.  So far so good on this feature, going to run the 
tests too.

Best,
Ryan

Re: Add default role 'pg_access_server_files'

[Ryan Murphy]

The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:   tested, passed
Spec compliant:   not tested
Documentation:tested, passed

I ran make installcheck-world and all tests passed except one that is a known 
issue with the way I have my environment setup (ecpg tests unrelated to this 
patch).

Manual tests I ran to see if it Implements the Feature:

1) confirmed that superuser can call pg_read_file() to read files in or out of 
data directory
2) confirmed that "tester" can call pg_read_file() only if given EXECUTE 
privilege
3) confirmed that "tester" can only call pg_read_file() on a file OUTSIDE of 
the data directory iff I "grant pg_access_server_files to tester;"

Documentation seems reasonable.

I believe this patch to be Ready for Committer.

The new status of this patch is: Ready for Committer

Re: [HACKERS] Creating backup history files for backups taken from standbys

[Michael Paquier]

On Fri, Jan 5, 2018 at 11:27 PM, Simon Riggs  wrote:
> I'm not.
>
> If we want to do this why not only do it in the modes that have meaning?
> i.e. put an if() test in for archive_mode == always.

OK, I can see value in your point as well. The check is a bit more
complicated that just looking for archive_mode == always though, you
need to look at if the backup has been started in recovery or not, and
then adapt using XLogArchivingActive() and XLogArchivingAlways(),
similarly to what is done when waiting for the archives.

> Which also makes it a smaller and clearer patch

Yes, this generates less diffs, reducing the likelihood of bugs. What
do you think about the v3 attached?
-- 
Michael


standby-backup-history-v3.patch
Description: Binary data

Re: [HACKERS] Removing useless DISTINCT clauses

[David Rowley]

Hi Jeff,

Thanks for looking at the patch.

On 6 January 2018 at 10:34, Jeff Janes  wrote:
> Couldn't the Unique node be removed entirely?  If k is a primary key, you
> can't have duplicates in need of removal.

It probably could be, if there were no joins, but any join could
duplicate those rows, so we'd only be able to do that if there were no
joins, and I'm not sure we'd want to special case that. It seems just
too naive to be taken seriously. We could do a lot better...

> Or would that be a subject for a different patch?

Yeah, I'd rather do that fix properly, and do have ideas on how to,
just it's a lot more work (basically testing if the joins can
duplicate rows or not (unique joins++)).

> I think remove_functionally_dependant_groupclauses should have a more
> generic name, like remove_functionally_dependant_clauses.

It's been a while since I looked at this. I remember thinking
something similar at the time but I must have not changed it.

I'll look again and post an updated patch.

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

Re: [HACKERS] Removing useless DISTINCT clauses

[David Rowley]

On 6 January 2018 at 23:08, David Rowley  wrote:
>> I think remove_functionally_dependant_groupclauses should have a more
>> generic name, like remove_functionally_dependant_clauses.
>
> It's been a while since I looked at this. I remember thinking
> something similar at the time but I must have not changed it.
>
> I'll look again and post an updated patch.

I've attached a patch that renames the function as you've suggested.

Thanks again for the review.

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


remove_useless_distinct_clauses_v2.patch
Description: Binary data

Re: FOR EACH ROW triggers on partitioned tables

[Simon Riggs]

On 3 January 2018 at 03:12, Peter Eisentraut
 wrote:
> On 12/29/17 17:53, Alvaro Herrera wrote:
>> This patch enables FOR EACH ROW triggers on partitioned tables.
>>
>> As presented, this patch is sufficient to discuss the semantics that we
>> want for triggers on partitioned tables, which is the most pressing
>> question here ISTM.
>
> This seems pretty straightforward.  What semantics questions do you have?

I see the patch imposes these restrictions

* AFTER TRIGGERS only

* No transition tables

* No WHEN clause

All of which might be removed/extended at some later date

So that's all good... there's not much here, so easy to commit soon.

>> However, this is incomplete: it doesn't create triggers when you do
>> ALTER TABLE ATTACH PARTITION or by CREATE TABLE PARTITION OF.  I'm using
>> this as a basis on which to try foreign keys for partitioned tables.
>> Getting this to committable status requires adding those features.
>
> Yeah that, and also perhaps preventing the removal of triggers from
> partitions if they are supposed to be on the whole partition hierarchy.

+1

> And then make pg_dump do the right things.  That's all mostly legwork, I
> think.
>
> Also, does ALTER TABLE ... ENABLE/DISABLE TRIGGER do the right things on
> partitioned tables?

Not sure I care about that, since it just breaks FKs and other things,
but we can add it later.

-- 
Simon Riggshttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] Removing [Merge]Append nodes which contain a single subpath

[David Rowley]

On 28 December 2017 at 18:31, David Rowley  wrote:
> I've attached a rebased patch.
>
> The previous patch was conflicting with parallel Hash Join (180428404)

And another one. Thanks to the patch tester [1], I realised that I
didn't make check-world and there was a postgres_fdw test that was
failing.

The attached fixes.

[1] http://commitfest.cputube.org/

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


remove_singleton_appends_2018-01-07.patch
Description: Binary data

Re: unique indexes on partitioned tables

[Simon Riggs]

On 29 December 2017 at 23:06, Alvaro Herrera  wrote:
> This is the patch series for UNIQUE / PRIMARY KEY indexes on partitioned
> tables.  This is on top of the patch in
> https://postgr.es/m/20171229175930.3aew7lzwd5w6m2x6@alvherre.pgsql
> but I included it here as 0001 for simplicity.  (Don't review that patch
> in this thread please).  This is essentially the same patch I posted
> elsewhere in that thread.

This is a very simple patch, so not much to object to. Most of the
code is about passing the details thru APIs.

Looks good to go.

The comments are slightly better than the explanation in the docs.

> I included Amit's support for ON CONFLICT DO UPDATE, but as I mentioned
> in the other thread, it has a small bug.  In principle we could push
> 0002 together with 0003, but I'd rather fix 0004 first and push it all
> as one commit.

I agree we want 0004 also, but it would be simpler to just push 0002
and 0003 now and come back later for 0004. That would also avoid any
confusion over patch credits.

> This serves as basis to build foreign keys on top; I'll post that
> separately.

-- 
Simon Riggshttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [PATCH] Logical decoding of TRUNCATE

[Marco Nenciarini]

Patch rebased on the current master.

Regards,
Marco

-- 
Marco Nenciarini - 2ndQuadrant Italy
PostgreSQL Training, Services and Support
marco.nenciar...@2ndquadrant.it | www.2ndQuadrant.it
diff --git a/contrib/test_decoding/expected/ddl.out 
b/contrib/test_decoding/expected/ddl.out
index 1e22c1eefc..d1feea4909 100644
*** a/contrib/test_decoding/expected/ddl.out
--- b/contrib/test_decoding/expected/ddl.out
***
*** 543,548  UPDATE table_with_unique_not_null SET data = 3 WHERE data = 2;
--- 543,550 
  UPDATE table_with_unique_not_null SET id = -id;
  UPDATE table_with_unique_not_null SET id = -id;
  DELETE FROM table_with_unique_not_null WHERE data = 3;
+ TRUNCATE table_with_unique_not_null;
+ TRUNCATE table_with_unique_not_null, table_with_unique_not_null RESTART 
IDENTITY CASCADE;
  -- check toast support
  BEGIN;
  CREATE SEQUENCE toasttable_rand_seq START 79 INCREMENT 1499; -- portable 
"random"
***
*** 660,665  SELECT data FROM 
pg_logical_slot_get_changes('regression_slot', NULL, NULL, 'inc
--- 662,673 
   table public.table_with_unique_not_null: DELETE: id[integer]:4
   COMMIT
   BEGIN
+  table public.table_with_unique_not_null: TRUNCATE: (no-flags)
+  COMMIT
+  BEGIN
+  table public.table_with_unique_not_null: TRUNCATE: restart_seqs cascade
+  COMMIT
+  BEGIN
   table public.toasttable: INSERT: id[integer]:1 
toasted_col1[text]:'12345678910111213141516171819202122232425262728293031323334353637383940414243444546474849505152535455565758596061626364656667686970717273747576777879808182838485868788899091929394959697989910010110210310410510610710810911012113114115116117118119120121122123124125126127128129130131132133134135136137138139140141142143144145146147148149150151152153154155156157158159160161162163164165166167168169170171172173174175176177178179180181182183184185186187188189190191192193194195196197198199200201202203204205206207208209210211212213214215216217218219220221232242252262272282292302312322332342352362372382392402412422432442452462472482492502512522532542552562572582592602612622632642652662672682692702712722732742752762772782792802812822832842852862872882892902912922932942952962972982993003013023033043053063073083093103113123133143153163173183193203213223233243253263273283293303313323433533633733833934034134234334434534634734834935035135235335435535635735835936036136236336436536636736836937037137237337437537637737837938038138238338438538638738838939039139239339439539639739839940040140240340440540640740840941041141241341441541641741841942042142242342442542642742842943043143243343443543643743843944044144244345446447448449450451452453454455456457458459460461462463464465466467468469470471472473474475476477478479480481482483484485486487488489490491492493494495496497498499500501502503504505506507508509510511512513514515516517518519520521522523524525526527528529530531532533534535536537538539540541542543544545546547548549550551552553554565575585595605615625635645655665675685695705715725735745755765775785795805815825835845855865875885895905915925935945955965975985996006016026036046056066076086096106116126136146156166176186196206216226236246256266276286296306316326336346356366376386396406416426436446456466476486496506516526536546556566576586596606616626636646656766866967067167267367467567667767867968068168268368468568668768868969069169269369469569669769869970070170270370470570670770870971071171271371471571671771871972072172272372472572672772872973073173273373473573673773873974074174274374474574674774874975075175275375475575675775875976076176276376476576676776876977077177277377477577678779780781782783784785786787788789790791792793794795796797798799800801802803804805806807808809810811812813814815816817818819820821822823824825826827828829830831832833834835836837838839840841842843844845846847848849850851852853854855856857858859860861862863864865866867868869870871872873874875876877878879880881882883884885886887898908918928938948958968978988999009019029039049059069079089099109119129139149159169179189199209219229239249259269279289299309319329339349359369379389399409419429439449459469479489499509519529539549559569579589599609619629639649659669679689699709719729739749759769779789799809819829839849859869879889899909919929939949959969979989991000100110021003100410051006100710081009101010111012101310141015101610171018101910201021102210231024102510261027102810291030103110321033103410351036103710381039104010411042104310441045104610471048104910501051105210531054105510561057105810591060106110621063106410651066106710681069107010711072107310741075107610771078107910801081108210831084108510861087108810891090109110921093109410951096109710981099110011011102110311041105110611071108110911101112111311141115111611171118111911201121112211231124112511261127112811291130113111321133113411351136113711381139114011411142114311441145114611471148114911501151115211531154115511561157115811591160116111621163116411651166116711681169117011711172117311741175117611771

Re: to_timestamp TZH and TZM format specifiers

[Andrew Dunstan]

On 01/03/2018 02:21 PM, Andrew Dunstan wrote:
>
> On 01/03/2018 01:34 PM, Tom Lane wrote:
>> Andrew Dunstan  writes:
>>> This small and simple standalone patch extracted from the SQL/JSON work
>>> would allow the user to supply a string with a time zone specified as
>>> hh:mm thus:
>>> SELECT to_timestamp('2011-12-18 11:38 -05:20', '-MM-DD HH12:MI
>>> TZH:TZM');
>>>  to_timestamp
>>> --
>>>  Sun Dec 18 08:58:00 2011 PST
>> I see that Oracle's to_timestamp supports these format codes, so +1
>> if you've checked that the behavior is compatible with Oracle.  The
>> most obvious possible gotcha is whether + is east or west of GMT,
>> but also there's formatting questions like what the field width is
>> and whether leading zeroes are printed.
>>
>> Also, I'm unimpressed that you've not bothered to implement the
>> to_char direction.  That moves this from a feature addition to
>> a kluge, IMO, especially since that ought to be the easier direction.
>>
>>
>
>
> To be clear, this isn't my patch, it one I extracted from the large
> patchset Nikita Glukhov posted for SQL/JSON, in order to kickstart
> process there.
>
> I wasn't aware of the Oracle implementation.
>
> I agree that supporting these in to_char would be useful, and should not
> be terribly difficult.
>
>


Here is a version that adds the to_char direction. AFAICT it is
compatible with Oracle.

cheers

andrew

-- 

Andrew Dunstanhttps://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 4dd9d02..2428434 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -6074,6 +6074,14 @@ SELECT regexp_match('abc01234xyz', '(?:(.*?)(\d+)(.*)){1,1}');
  (only supported in to_char)


+   TZH
+time-zone hours
+   
+   
+   TZM
+time-zone minutes
+   
+   
 OF
 time-zone offset from UTC
  (only supported in to_char)
diff --git a/src/backend/utils/adt/formatting.c b/src/backend/utils/adt/formatting.c
index 0e30810..b8bd4ca 100644
--- a/src/backend/utils/adt/formatting.c
+++ b/src/backend/utils/adt/formatting.c
@@ -424,7 +424,10 @@ typedef struct
 j,
 us,
 yysz,			/* is it YY or  ? */
-clock;			/* 12 or 24 hour clock? */
+clock,			/* 12 or 24 hour clock? */
+tzsign,			/* +1, -1 or 0 if timezone info is absent */
+tzh,
+tzm;
 } TmFromChar;
 
 #define ZERO_tmfc(_X) memset(_X, 0, sizeof(TmFromChar))
@@ -470,6 +473,7 @@ do {	\
 	(_X)->tm_sec  = (_X)->tm_year = (_X)->tm_min = (_X)->tm_wday = \
 	(_X)->tm_hour = (_X)->tm_yday = (_X)->tm_isdst = 0; \
 	(_X)->tm_mday = (_X)->tm_mon  = 1; \
+	(_X)->tm_zone = NULL; \
 } while(0)
 
 #define ZERO_tmtc(_X) \
@@ -609,6 +613,8 @@ typedef enum
 	DCH_RM,
 	DCH_,
 	DCH_SS,
+	DCH_TZH,
+	DCH_TZM,
 	DCH_TZ,
 	DCH_US,
 	DCH_WW,
@@ -756,7 +762,9 @@ static const KeyWord DCH_keywords[] = {
 	{"RM", 2, DCH_RM, false, FROM_CHAR_DATE_GREGORIAN}, /* R */
 	{"", 4, DCH_, true, FROM_CHAR_DATE_NONE},	/* S */
 	{"SS", 2, DCH_SS, true, FROM_CHAR_DATE_NONE},
-	{"TZ", 2, DCH_TZ, false, FROM_CHAR_DATE_NONE},	/* T */
+	{"TZH", 3, DCH_TZH, false, FROM_CHAR_DATE_NONE},	/* T */
+	{"TZM", 3, DCH_TZM, true, FROM_CHAR_DATE_NONE},
+	{"TZ", 2, DCH_TZ, false, FROM_CHAR_DATE_NONE},
 	{"US", 2, DCH_US, true, FROM_CHAR_DATE_NONE},	/* U */
 	{"WW", 2, DCH_WW, true, FROM_CHAR_DATE_GREGORIAN},	/* W */
 	{"W", 1, DCH_W, true, FROM_CHAR_DATE_GREGORIAN},
@@ -879,7 +887,7 @@ static const int DCH_index[KeyWord_INDEX_SIZE] = {
 	-1, -1, -1, -1, -1, -1, -1, -1, -1, -1,
 	-1, -1, -1, -1, -1, DCH_A_D, DCH_B_C, DCH_CC, DCH_DAY, -1,
 	DCH_FX, -1, DCH_HH24, DCH_IDDD, DCH_J, -1, -1, DCH_MI, -1, DCH_OF,
-	DCH_P_M, DCH_Q, DCH_RM, DCH_, DCH_TZ, DCH_US, -1, DCH_WW, -1, DCH_Y_YYY,
+	DCH_P_M, DCH_Q, DCH_RM, DCH_, DCH_TZH, DCH_US, -1, DCH_WW, -1, DCH_Y_YYY,
 	-1, -1, -1, -1, -1, -1, -1, DCH_a_d, DCH_b_c, DCH_cc,
 	DCH_day, -1, DCH_fx, -1, DCH_hh24, DCH_iddd, DCH_j, -1, -1, DCH_mi,
 	-1, -1, DCH_p_m, DCH_q, DCH_rm, DCH_, DCH_tz, DCH_us, -1, DCH_ww,
@@ -2519,6 +2527,19 @@ DCH_to_char(FormatNode *node, bool is_interval, TmToChar *in, char *out, Oid col
 	s += strlen(s);
 }
 break;
+			case DCH_TZH:
+INVALID_FOR_INTERVAL;
+sprintf(s, "%c%02d",
+		(tm->tm_gmtoff >= 0) ? '+' : '-',
+		abs((int) tm->tm_gmtoff) / SECS_PER_HOUR);
+s += strlen(s);
+break;
+			case DCH_TZM:
+INVALID_FOR_INTERVAL;
+sprintf(s, "%02d",
+		(abs((int) tm->tm_gmtoff) % SECS_PER_HOUR) / SECS_PER_MINUTE);
+s += strlen(s);
+break;
 			case DCH_OF:
 INVALID_FOR_INTERVAL;
 sprintf(s, "%c%0*d",
@@ -3070,6 +3091,20 @@ DCH_from_char(FormatNode *node, char *in, TmFromChar *out)
 		 errmsg("formatting field \"%s\" is only supported in to_char",
 n->key->name)));
 break;
+			case DCH_TZH:
+out->tzsign = *s =

Re: [HACKERS] Creating backup history files for backups taken from standbys

[David Steele]

On 1/6/18 3:48 AM, Michael Paquier wrote:

On Fri, Jan 5, 2018 at 11:27 PM, Simon Riggs  wrote:


Which also makes it a smaller and clearer patch


Yes, this generates less diffs, reducing the likelihood of bugs. What
do you think about the v3 attached?


I agree that this is a cleaner solution.

--
-David
da...@pgmasters.net

Re: Challenges preventing us moving to 64 bit transaction id (XID)?

[Ryan Murphy]

Since the Patch Tester (http://commitfest.cputube.org/) says this Patch will 
not apply correctly, I am tempted to change the status to Waiting on Author.

However, I'm new to the CommitFest process, so I'm leaving the status as-is for 
now and asking Stephen Frost whether he agrees.

I haven't tried to apply the patch myself yet, but happy to do so if e.g. we 
think the Patch Tester can't be taken on face value,
or if I need to find specific feedback about why the patch didn't apply to help 
the Author.

Best regards,
Ryan

Re: Rangejoin rebased

[Simon Riggs]

On 29 December 2017 at 18:25, Jeff Davis  wrote:
> New rangejoin patch attached.
>
> I had previously attempted to make this work well for multiple range
> join keys, but this patch implements single rangejoin keys only, and
> the rest of the rangejoin clauses are effectively just rechecks. I
> believe it can be made effective for multiple rangejoin keys, but at
> the cost of additional complexity which is neither justified nor
> implemented at this point.

For this to be useful, it needs to include some details of how to use
it when people have NOT used range datatypes in their tables.

If we can see some examples with StartDate and EndDate cast to a
tzrange, plus some "don't write it like this" anti-patterns that would
help.

We can then make it clear that this is a huge performance benefit for
these important cases.

Just to emphasise why we want this, it might be better for the EXPLAIN
to say "Time Range Join" when the ranges being joined are Time Ranges,
and for other cases to just say "Range Join". The use of the word
Merge doesn't help much there.

-- 
Simon Riggshttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Challenges preventing us moving to 64 bit transaction id (XID)?

[Ryan Murphy]

Thanks for this contribution!
I think it's a hard but important problem to upgrade these xids.

Unfortunately, I've confirmed that this patch 0001-64bit-guc-relopt-3.patch 
doesn't apply correctly on my computer.

Here's what I did:

I did a "git pull" to the current HEAD, which is 
6271fceb8a4f07dafe9d67dcf7e849b319bb2647

Then I attempted to apply the patch, here's what I saw:

$ git apply patches/0001-64bit-guc-relopt-3.patch 
error: src/backend/access/common/reloptions.c: already exists in working 
directory
error: src/backend/utils/misc/guc.c: already exists in working directory
error: src/include/access/reloptions.h: already exists in working directory
error: src/include/utils/guc.h: already exists in working directory
error: src/include/utils/guc_tables.h: already exists in working directory

Alexander, what is the process you're using to create the patch?  I've heard 
someone (maybe Tom Lane?) say that he sometimes uses "patch" directly instead 
of "git" to create the patch, with better results.  I forget the exact command.

For now I'm setting this to Waiting on Author, until we have a patch that 
applies via "git apply".

Thanks!
Ryan

The new status of this patch is: Waiting on Author

Re: [PATCH] Logical decoding of TRUNCATE

[Marco Nenciarini]

Attached here there is the complete list of patches required to pass all
the tests. The 0001 patch is discussed in a separate thread, but I've
posted it also here to ease the review of the 0002.

Regards,
Marco

-- 
Marco Nenciarini - 2ndQuadrant Italy
PostgreSQL Training, Services and Support
marco.nenciar...@2ndquadrant.it | www.2ndQuadrant.it
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index e4a01699e4..aff56891e6 100644
*** a/doc/src/sgml/config.sgml
--- b/doc/src/sgml/config.sgml
***
*** 6502,6508  COPY postgres_log FROM '/full/path/to/logfile.csv' WITH csv;

 
  Controls firing of replication-related triggers and rules for the
! current session.  Setting this variable requires
  superuser privilege and results in discarding any previously cached
  query plans.  Possible values are origin (the 
default),
  replica and local.
--- 6502,6510 

 
  Controls firing of replication-related triggers and rules for the
! current session. When set to replica it also
! disables all the foreign key checks, which can leave the data in an
! inconsistent state if improperly used. Setting this variable requires
  superuser privilege and results in discarding any previously cached
  query plans.  Possible values are origin (the 
default),
  replica and local.
diff --git a/src/backend/commanindex d979ce266d..296807849f 100644
*** a/src/backend/commands/tablecmds.c
--- b/src/backend/commands/tablecmds.c
***
*** 1341,1356  ExecuteTruncate(TruncateStmt *stmt)
}
  
/*
!* Check foreign key references.  In CASCADE mode, this should be
!* unnecessary since we just pulled in all the references; but as a
!* cross-check, do it anyway if in an Assert-enabled build.
 */
  #ifdef USE_ASSERT_CHECKING
-   heap_truncate_check_FKs(rels, false);
- #else
-   if (stmt->behavior == DROP_RESTRICT)
heap_truncate_check_FKs(rels, false);
  #endif
  
/*
 * If we are asked to restart sequences, find all the sequences, lock 
them
--- 1341,1364 
}
  
/*
!* Suppress foreign key references check if session replication role is
!* set to REPLICA.
 */
+   if (SessionReplicationRole != SESSION_REPLICATION_ROLE_REPLICA)
+   {
+ 
+   /*
+* Check foreign key references.  In CASCADE mode, this should 
be
+* unnecessary since we just pulled in all the references; but 
as a
+* cross-check, do it anyway if in an Assert-enabled build.
+*/
  #ifdef USE_ASSERT_CHECKING
heap_truncate_check_FKs(rels, false);
+ #else
+   if (stmt->behavior == DROP_RESTRICT)
+   heap_truncate_check_FKs(rels, false);
  #endif
+   }
  
/*
 * If we are asked to restart sequences, find all the sequences, lock 
them
diff --git a/contrib/test_decoding/expected/ddl.out 
b/contrib/test_decoding/expected/ddl.out
index 1e22c1eefc..d1feea4909 100644
*** a/contrib/test_decoding/expected/ddl.out
--- b/contrib/test_decoding/expected/ddl.out
***
*** 543,548  UPDATE table_with_unique_not_null SET data = 3 WHERE data = 2;
--- 543,550 
  UPDATE table_with_unique_not_null SET id = -id;
  UPDATE table_with_unique_not_null SET id = -id;
  DELETE FROM table_with_unique_not_null WHERE data = 3;
+ TRUNCATE table_with_unique_not_null;
+ TRUNCATE table_with_unique_not_null, table_with_unique_not_null RESTART 
IDENTITY CASCADE;
  -- check toast support
  BEGIN;
  CREATE SEQUENCE toasttable_rand_seq START 79 INCREMENT 1499; -- portable 
"random"
***
*** 660,665  SELECT data FROM 
pg_logical_slot_get_changes('regression_slot', NULL, NULL, 'inc
--- 662,673 
   table public.table_with_unique_not_null: DELETE: id[integer]:4
   COMMIT
   BEGIN
+  table public.table_with_unique_not_null: TRUNCATE: (no-flags)
+  COMMIT
+  BEGIN
+  table public.table_with_unique_not_null: TRUNCATE: restart_seqs cascade
+  COMMIT
+  BEGIN
   table public.toasttable: INSERT: id[integer]:1 
toasted_col1[text]:'123456789101112131415161718192021222324252627282930313233343536373839404142434445464748495051525354555657585960616263646566676869707172737475767778798081828384858687888990919293949596979899100101102103104105106107108109110121131141151161171181191201211221231241251261271281291301311321331341351361371381391401411421431441451461471481491501511521531541551561571581591601611621631641651661671681691701711721731741751761771781791801811821831841851861871881891901911921931941951961971981992002012022032042052062072082092102112122132142152162172182192202212322422522622722822923023123223323423523623723823924024124224324424524624724824925025125225325425525625725825926026126226326426526626726826927027127227327427527627727827928028128228328

Re: Add default role 'pg_access_server_files'

[Ryan Murphy]

Stephen, so far I've read thru your patch and familiarized myself with some of 
the auth functionality in pg_authid.h and src/backend/utils/adt/acl.c

The only question I have so far about your patch is the last several hunks of 
the diff, which remove superuser checks without adding anything immediately 
obvious in their place:

...
@@ -195,11 +205,6 @@ pg_read_file(PG_FUNCTION_ARGS)
char   *filename;
text   *result;
 
-   if (!superuser())
-   ereport(ERROR,
-   (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
-(errmsg("must be superuser to read files";
-
/* handle optional arguments */
if (PG_NARGS() >= 3)
{
@@ -236,11 +241,6 @@ pg_read_binary_file(PG_FUNCTION_ARGS)
char   *filename;
bytea  *result;
 
-   if (!superuser())
-   ereport(ERROR,
-   (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
-(errmsg("must be superuser to read files";
-
/* handle optional arguments */
if (PG_NARGS() >= 3)
{
@@ -313,11 +313,6 @@ pg_stat_file(PG_FUNCTION_ARGS)
TupleDesc   tupdesc;
boolmissing_ok = false;
 
-   if (!superuser())
-   ereport(ERROR,
-   (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
-(errmsg("must be superuser to get file information";
-
/* check the optional argument */
if (PG_NARGS() == 2)
missing_ok = PG_GETARG_BOOL(1);
...

I wanted to ask if you have reason to believe that these checks were not 
necessary (and therefore can be deleted instead of replaced by 
is_member_of_role() checks like you did elsewhere).  I still have limited 
understanding of the overall code, so really just asking because it's the first 
thing that jumped out.

Best,
Ryan

Re: [HACKERS] [PROPOSAL] Temporal query processing with range types

[Simon Riggs]

On 30 November 2017 at 17:26, Robert Haas  wrote:

> I wonder if we could instead think about R NORMALIZE S ON R.x = S.y
> WITH (R.time, S.time) as an ordinary join on R.x = S.y with some extra
> processing afterwards.

That would work nicely, kindof like a Projection, but one that can
vary the number of rows emitted.

For normal joins, we simply emit one row. For new style joins we call
a special PostJoinSetProjection function: one tuple in, potentially
many tuples out.

Peter, does Robert's proposed treatment give you what you need?


Overall, I like the goals expressed on this thread. I think if we
should focus on introducing useful new functionality, rather than
focusing on syntax.

I'm not very keen on adopting new syntax that isn't in the
SQLStandard. They have a bad habit of doing something completely
different. So a flexible approach will allow us to have functionality
now and we can adopt any new syntax later.

For any new syntax, I think the right approach would be to create a
new parser plugin. That way we could make all of this part of an
extension.
* a parser plugin for any new syntax
* various PostJoinSetProjection() functions to be called as needed
So the idea is we enable Postgres to allow major new functionality, as
was done for PostGIS so successfully.

We can adopt syntax into the main parser later once SQLStandard
accepts this, or some munged version of it.

-- 
Simon Riggshttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Add default role 'pg_access_server_files'

[Stephen Frost]

Greetings Ryan!

* Ryan Murphy (ryanfmur...@gmail.com) wrote:
> Stephen, so far I've read thru your patch and familiarized myself with some 
> of the auth functionality in pg_authid.h and src/backend/utils/adt/acl.c
> 
> The only question I have so far about your patch is the last several hunks of 
> the diff, which remove superuser checks without adding anything immediately 
> obvious in their place:

Ah, I realize it's not immediately obvious, but they *are* replaced by
something else- REVOKE statements in the "system_views.sql" file in
src/backend/catalog:

REVOKE EXECUTE ON FUNCTION pg_read_file(text) FROM public;
REVOKE EXECUTE ON FUNCTION pg_read_file(text,bigint,bigint) FROM public;
REVOKE EXECUTE ON FUNCTION pg_read_file(text,bigint,bigint,boolean) FROM public;

REVOKE EXECUTE ON FUNCTION pg_read_binary_file(text) FROM public;
REVOKE EXECUTE ON FUNCTION pg_read_binary_file(text,bigint,bigint) FROM public;
REVOKE EXECUTE ON FUNCTION pg_read_binary_file(text,bigint,bigint,boolean) FROM 
public;

REVOKE EXECUTE ON FUNCTION pg_stat_file(text) FROM public;
REVOKE EXECUTE ON FUNCTION pg_stat_file(text,boolean) FROM public;

That script is run as part of 'initdb' to set things up in the system.
By issueing those REVOKE statements, no one but the cluster owner (a
superuser) is able to run those functions- unless a superuser decides
that it's ok for others to run them, in which case they would run:

GRANT EXECUTE ON FUNCTION pg_read_file(text) TO myuser;

> I wanted to ask if you have reason to believe that these checks were not 
> necessary (and therefore can be deleted instead of replaced by 
> is_member_of_role() checks like you did elsewhere).  I still have limited 
> understanding of the overall code, so really just asking because it's the 
> first thing that jumped out.

The places where is_member_of_role() is being used are cases where it's
not possible to use the GRANT system.  For example, we don't have a way
to say "GRANT read-files-outside-the-data-directory TO role1;" in the
normal GRANT system, and so a default role is added to allow that
specific right to be GRANT'd to non-superuser.

One would need to have both the default role AND EXECUTE rights on the
function to be able to, say, run:

SELECT pg_read_file('/data/load_area/load_file');

With just EXECUTE on the function, they could use pg_read_file() to read
files under the data directory but not elsewhere on the system, which
may be overly limiting for some use-cases.

Of course, all of these functions allow a great deal of access to the
system and that's why they started out being superuser-only.
Administrators will need to carefully consider who, if anyone, should
have the level of access which these functions and default roles
provide.

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: Condition variable live lock

[Tom Lane]

I wrote:
> Robert Haas  writes:
>> No opinion without seeing what you propose to change.

> OK, will put out a proposal.

I began with the intention of making no non-cosmetic changes, but then
I started to wonder why ConditionVariablePrepareToSleep bothers with a
!proclist_contains test, when the calling process surely ought not be
in the list -- or any other list -- since it wasn't prepared to sleep.
And that led me down a different rabbit hole ending in the conclusion
that proclist.h could stand some improvements too.  I do not like the
fact that it's impossible to tell whether a proclist_node is in any
proclist or not.  Initially, a proclist_node contains zeroes which is
a distinguishable state, but proclist_delete_offset resets it to
next = prev = INVALID_PGPROCNO which looks the same as a node that's in a
singleton list.  We should have it reset to the initial state of zeroes
instead, and then we can add assertions to proclist_push_xxx that the
supplied node is not already in a list.  Hence, I propose the first
attached patch which tightens things up in proclist.h and then removes
the !proclist_contains test in ConditionVariablePrepareToSleep; the
assertion in proclist_push_tail supersedes that.

The second attached patch is the cosmetic changes I want to make in
condition_variable.c/.h.

I still think that we ought to change the Asserts on cv_sleep_target in
ConditionVariablePrepareToSleep and ConditionVariableSleep to be full
test-and-elog tests.  Those are checking a global correctness property
("global" meaning "interactions between completely unrelated modules can
break this"), and they'd be extremely cheap compared to the rest of what
those functions are doing, so I think insisting that they be Asserts is
penny wise and pound foolish.  Anybody besides Robert want to vote on
that?

Another loose end that I'm seeing here is that while a process waiting on
a condition variable will respond to a cancel or die interrupt, it will
not notice postmaster death.  This seems unwise to me.  I think we should
adjust the WaitLatch call to include WL_POSTMASTER_DEATH as a wake
condition and just do a summary proc_exit(1) if it sees that.  I'd even
argue that that is a back-patchable bug fix.

regards, tom lane

diff --git a/src/include/storage/proclist_types.h b/src/include/storage/proclist_types.h
index 237fb76..f4dac10 100644
--- a/src/include/storage/proclist_types.h
+++ b/src/include/storage/proclist_types.h
@@ -16,7 +16,12 @@
 #define PROCLIST_TYPES_H
 
 /*
- * A node in a list of processes.
+ * A node in a doubly-linked list of processes.  The link fields contain
+ * the 0-based PGPROC indexes of the next and previous process, or
+ * INVALID_PGPROCNO in the next-link of the last node and the prev-link
+ * of the first node.  A node that is currently not in any list
+ * should have next == prev == 0; this is not a possible state for a node
+ * that is in a list, because we disallow circularity.
  */
 typedef struct proclist_node
 {
@@ -25,7 +30,8 @@ typedef struct proclist_node
 } proclist_node;
 
 /*
- * Head of a doubly-linked list of PGPROCs, identified by pgprocno.
+ * Header of a doubly-linked list of PGPROCs, identified by pgprocno.
+ * An empty list is represented by head == tail == INVALID_PGPROCNO.
  */
 typedef struct proclist_head
 {
@@ -42,4 +48,4 @@ typedef struct proclist_mutable_iter
 	int			next;			/* pgprocno of the next PGPROC */
 } proclist_mutable_iter;
 
-#endif
+#endif			/* PROCLIST_TYPES_H */
diff --git a/src/include/storage/proclist.h b/src/include/storage/proclist.h
index 4e25b47..59a478e 100644
--- a/src/include/storage/proclist.h
+++ b/src/include/storage/proclist.h
@@ -42,7 +42,7 @@ proclist_is_empty(proclist_head *list)
 
 /*
  * Get a pointer to a proclist_node inside a given PGPROC, given a procno and
- * an offset.
+ * the proclist_node field's offset within struct PGPROC.
  */
 static inline proclist_node *
 proclist_node_get(int procno, size_t node_offset)
@@ -53,13 +53,15 @@ proclist_node_get(int procno, size_t node_offset)
 }
 
 /*
- * Insert a node at the beginning of a list.
+ * Insert a process at the beginning of a list.
  */
 static inline void
 proclist_push_head_offset(proclist_head *list, int procno, size_t node_offset)
 {
 	proclist_node *node = proclist_node_get(procno, node_offset);
 
+	Assert(node->next == 0 && node->prev == 0);
+
 	if (list->head == INVALID_PGPROCNO)
 	{
 		Assert(list->tail == INVALID_PGPROCNO);
@@ -79,13 +81,15 @@ proclist_push_head_offset(proclist_head *list, int procno, size_t node_offset)
 }
 
 /*
- * Insert a node at the end of a list.
+ * Insert a process at the end of a list.
  */
 static inline void
 proclist_push_tail_offset(proclist_head *list, int procno, size_t node_offset)
 {
 	proclist_node *node = proclist_node_get(procno, node_offset);
 
+	Assert(node->next == 0 && node->prev == 0);
+
 	if (list->tail == INVALID_PGPROCNO)
 	{
 		Assert(list->head == INVALID_PGPROCNO);
@@ -105,30 +109,38 @@ procli

Re: [HACKERS] SQL/JSON in PostgreSQL

[Oleg Bartunov]

On Sat, Jan 6, 2018 at 8:22 AM, Pavel Stehule  wrote:
> Hi
>
> I am checking the JSONPath related code
>
> Questions, notes:
>
> 1. jsonpath operators are not consistent with any other .. json, xml .. I am
> missing ?, @> operátors

I have slides about jsonpath
http://www.sai.msu.su/~megera/postgres/talks/sqljson-pgconf.eu-2017.pdf

> 2. documentation issue - there is "'{"a":[1,2,3,4,5]}'::json *? '$.a[*] ? (@
>> 2)'" - operator *? doesn't exists

There are should be @? operator

> 3. operator @~ looks like too aggressive shortcut - should be better
> commented
>
> What is not clean, if jsonpath should to create some new operators for json,
> jsonb types? It is special filter, defined by type, so from my perspective
> the special operators are not necessary.

It's impossible to distinguish jsonpath from text, so introducing new operators
are easier than everytime explicitly specify jsonpath datatype.

>
> Regards
>
> Pavel
>
>

Re: Challenges preventing us moving to 64 bit transaction id (XID)?

[Tom Lane]

Ryan Murphy  writes:
> Alexander, what is the process you're using to create the patch?  I've heard 
> someone (maybe Tom Lane?) say that he sometimes uses "patch" directly instead 
> of "git" to create the patch, with better results.  I forget the exact 
> command.

Nah, you've got that the other way 'round.  "patch" is not for creating
patches, it's for applying them.  I've found, and some other people seem
to agree, that "patch" is more robust at applying patches than "git apply"
is.  You might try this for a patch created with "git diff":

patch -p1 

Re: [HACKERS] SQL/JSON in PostgreSQL

[Pavel Stehule]

2018-01-06 22:02 GMT+01:00 Oleg Bartunov :

> On Sat, Jan 6, 2018 at 8:22 AM, Pavel Stehule 
> wrote:
> > Hi
> >
> > I am checking the JSONPath related code
> >
> > Questions, notes:
> >
> > 1. jsonpath operators are not consistent with any other .. json, xml ..
> I am
> > missing ?, @> operátors
>
> I have slides about jsonpath
> http://www.sai.msu.su/~megera/postgres/talks/sqljson-pgconf.eu-2017.pdf
>
> > 2. documentation issue - there is "'{"a":[1,2,3,4,5]}'::json *? '$.a[*]
> ? (@
> >> 2)'" - operator *? doesn't exists
>
> There are should be @? operator
>
> > 3. operator @~ looks like too aggressive shortcut - should be better
> > commented
> >
> > What is not clean, if jsonpath should to create some new operators for
> json,
> > jsonb types? It is special filter, defined by type, so from my
> perspective
> > the special operators are not necessary.
>
> It's impossible to distinguish jsonpath from text, so introducing new
> operators
> are easier than everytime explicitly specify jsonpath datatype.
>

There are two possible solutions - special operator or explicit casting. In
this case I am not sure if special operator for this case is good solution.
Probably nobody will use it - because there SQL/JSON functions, but I don't
think so this inconsistency is correct.

I have not strong opinion about it - it will be hidden feature for almost
all users.


> >
> > Regards
> >
> > Pavel
> >
> >
>

Re: [HACKERS] SQL/JSON in PostgreSQL

[Pavel Stehule]

Hi

I try jsonpath on json

{
"book":
[
{
"title": "Beginning JSON",
"author": "Ben Smith",
"price": 49.99
},

{
"title": "JSON at Work",
"author": "Tom Marrs",
"price": 29.99
},

{
"title": "Learn JSON in a DAY",
"author": "Acodemy",
"price": 8.99
},

{
"title": "JSON: Questions and Answers",
"author": "George Duckett",
"price": 6.00
}
],

"price range":
{
"cheap": 10.00,
"medium": 20.00
}
}


I am not jsonpath expert, so I can be bad

How I can get title of book with cost 6?

postgres=# select j @* '$.book[*] ? (@.price==6)' from test;
┌─┐
│  ?column?   │
╞═╡
│ {  ↵│
│ "title": "JSON: Questions and Answers",↵│
│ "author": "George Duckett",↵│
│ "price": 6.00  ↵│
│ }  ↵│
│ │
└─┘
(1 row)

-- not sure, if it is correct
postgres=# select j @* '$.book[*].title ? (@.price==6)' from test;
┌──┐
│ ?column? │
╞══╡
└──┘
(0 rows)

I found some examples, where the filter has bigger sense, but it is not
supported


LINE 1: select j @* '$.book[?(@.price==6.00)].title' from test;
^
DETAIL:  syntax error, unexpected '?' at or near "?"

Regards

Pavel

Re: [HACKERS] SQL/JSON in PostgreSQL

[Nikita Glukhov]

On 07.01.2018 00:22, Pavel Stehule wrote:


Hi

I try jsonpath on json

{
    "book":
    [
    {
    "title": "Beginning JSON",
    "author": "Ben Smith",
    "price": 49.99
    },

    {
    "title": "JSON at Work",
    "author": "Tom Marrs",
    "price": 29.99
    },

    {
    "title": "Learn JSON in a DAY",
    "author": "Acodemy",
    "price": 8.99
    },

    {
    "title": "JSON: Questions and Answers",
    "author": "George Duckett",
    "price": 6.00
    }
    ],

    "price range":
    {
    "cheap": 10.00,
    "medium": 20.00
    }
}


I am not jsonpath expert, so I can be bad

How I can get title of book with cost 6?

postgres=# select j @* '$.book[*] ? (@.price==6)' from test;
┌─┐
│  ?column?   │
╞═╡
│ {  ↵│
│ "title": "JSON: Questions and Answers",↵│
│ "author": "George Duckett",    ↵│
│ "price": 6.00  ↵│
│ }  ↵│
│ │
└─┘
(1 row)

-- not sure, if it is correct
postgres=# select j @* '$.book[*].title ? (@.price==6)' from test;
┌──┐
│ ?column? │
╞══╡
└──┘
(0 rows)

I found some examples, where the filter has bigger sense, but it is 
not supported



LINE 1: select j @* '$.book[?(@.price==6.00)].title' from test;
    ^
DETAIL:  syntax error, unexpected '?' at or near "?"


".title" simply should go after the filter:

select j @* '$.book[*] ? (@.price==6.00).title' from test;


--
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: [HACKERS] SQL/JSON in PostgreSQL

[Pavel Stehule]

2018-01-06 22:23 GMT+01:00 Nikita Glukhov :

> On 07.01.2018 00:22, Pavel Stehule wrote:
>
> Hi
>
> I try jsonpath on json
>
> {
> "book":
> [
> {
> "title": "Beginning JSON",
> "author": "Ben Smith",
> "price": 49.99
> },
>
> {
> "title": "JSON at Work",
> "author": "Tom Marrs",
> "price": 29.99
> },
>
> {
> "title": "Learn JSON in a DAY",
> "author": "Acodemy",
> "price": 8.99
> },
>
> {
> "title": "JSON: Questions and Answers",
> "author": "George Duckett",
> "price": 6.00
> }
> ],
>
> "price range":
> {
> "cheap": 10.00,
> "medium": 20.00
> }
> }
>
>
> I am not jsonpath expert, so I can be bad
>
> How I can get title of book with cost 6?
>
> postgres=# select j @* '$.book[*] ? (@.price==6)' from test;
> ┌─┐
> │  ?column?   │
> ╞═╡
> │ {  ↵│
> │ "title": "JSON: Questions and Answers",↵│
> │ "author": "George Duckett",↵│
> │ "price": 6.00  ↵│
> │ }  ↵│
> │ │
> └─┘
> (1 row)
>
> -- not sure, if it is correct
> postgres=# select j @* '$.book[*].title ? (@.price==6)' from test;
> ┌──┐
> │ ?column? │
> ╞══╡
> └──┘
> (0 rows)
>
> I found some examples, where the filter has bigger sense, but it is not
> supported
>
>
> LINE 1: select j @* '$.book[?(@.price==6.00)].title' from test;
> ^
> DETAIL:  syntax error, unexpected '?' at or near "?"
>
> ".title" simply should go after the filter:
>
> select j @* '$.book[*] ? (@.price==6.00).title' from test;
>
>
It is working, thank you.

and the form "$.book[?(@.price==6.00)].title" ? I found this example in
some other SQL/JSON implementations.



> --
> Nikita Glukhov
> Postgres Professional: http://www.postgrespro.com
> The Russian Postgres Company
>

Re: [HACKERS] SQL/JSON in PostgreSQL

[Nikita Glukhov]

On 07.01.2018 00:33, Pavel Stehule wrote:

2018-01-06 22:23 GMT+01:00 Nikita Glukhov >:


On 07.01.2018 00:22, Pavel Stehule wrote:


Hi

I try jsonpath on json

{
    "book":
    [
    {
    "title": "Beginning JSON",
    "author": "Ben Smith",
    "price": 49.99
    },

    {
    "title": "JSON at Work",
    "author": "Tom Marrs",
    "price": 29.99
    },

    {
    "title": "Learn JSON in a DAY",
    "author": "Acodemy",
    "price": 8.99
    },

    {
    "title": "JSON: Questions and Answers",
    "author": "George Duckett",
    "price": 6.00
    }
    ],

    "price range":
    {
    "cheap": 10.00,
    "medium": 20.00
    }
}


I am not jsonpath expert, so I can be bad

How I can get title of book with cost 6?

postgres=# select j @* '$.book[*] ? (@.price==6)' from test;
┌─┐
│ ?column?   │
╞═╡
│ { ↵│
│ "title": "JSON: Questions and Answers",↵│
│ "author": "George Duckett",    ↵│
│ "price": 6.00  ↵│
│ } ↵│
│ │
└─┘
(1 row)

-- not sure, if it is correct
postgres=# select j @* '$.book[*].title ? (@.price==6)' from test;
┌──┐
│ ?column? │
╞══╡
└──┘
(0 rows)

I found some examples, where the filter has bigger sense, but it
is not supported


LINE 1: select j @* '$.book[?(@.price==6.00)].title' from test;
    ^
DETAIL:  syntax error, unexpected '?' at or near "?"


".title" simply should go after the filter:

select j @* '$.book[*] ? (@.price==6.00).title' from test;


It is working, thank you.

and the form "$.book[?(@.price==6.00)].title" ? I found this example 
in some other SQL/JSON implementations.


This is non-standard feature, but it can be easily added for 
compatibility with other implementations.


--
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: Condition variable live lock

[Tom Lane]

I wrote:
> I still think that we ought to change the Asserts on cv_sleep_target in
> ConditionVariablePrepareToSleep and ConditionVariableSleep to be full
> test-and-elog tests.  Those are checking a global correctness property
> ("global" meaning "interactions between completely unrelated modules can
> break this"), and they'd be extremely cheap compared to the rest of what
> those functions are doing, so I think insisting that they be Asserts is
> penny wise and pound foolish.

Actually ... perhaps a better design would be to have
ConditionVariable[PrepareTo]Sleep auto-cancel any prepared sleep for
a different condition variable, analogously to what we just did in
ConditionVariableBroadcast, on the same theory that whenever control
returns to the other CV wait loop it can re-establish the relevant
state easily enough.  I have to think that if the use of CVs grows
much, the existing restriction is going to become untenable anyway,
so why not just get rid of it?

regards, tom lane

Re: PATCH: pgbench - option to build using ppoll() for larger connection counts

[Stephen Frost]

Doug,

* Rady, Doug (radyd...@amazon.com) wrote:
> This patch enables building pgbench to use ppoll() instead of select()
> to allow for more than (FD_SETSIZE - 10) connections.  As implemented,
> when using ppoll(), the only connection limitation is system resources.

Looks like this patch had some good feedback back when it was posted,
but we haven't seen any update to it based on that feedback and we're
now nearly a week into the January commitfest.

In order to keep the commitfest moving and to give you the best
opportunity at having the patch be reviewed and committed during this
cycle, I'd suggest trying to find some time soon to incorporate the
recommendations provided and post an updated patch for review.

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: General purpose hashing func in pgbench

[Stephen Frost]

Greetings Ildar,

* Fabien COELHO (coe...@cri.ensmp.fr) wrote:
> >>I noticed from the source of all human knowledege (aka Wikipedia:-)
> >>that there seems to be a murmur3 successor. Have you considered it?
> >>One good reason to skip it would be that the implementation is long
> >>and complex. I'm not sure about a 8-byte input simplified version.
> >Murmur2 naturally supports 8-byte data. Murmur3 has 32- and 128-bit
> >versions.
> 
> Ok.
> 
> So it would make sense to keep the 64 bit murmur2 version.
> 
> Pgbench ints are 64 bits, seems logical to keep them that way, so 32 bits
> versions do not look too interesting.

This sounds like it was settled and the patch needs updating with these
changes, and with documentation and tests added.

While the commitfest is still pretty early on, I would suggest trying to
find time soon to update the patch and resubmit it, so it can get
another review and hopefully be included during this commitfest.

Also, I've not checked myself, but the patch appears to be failing for
http://commitfest.cputube.org/ so it likely also needs a rebase.

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: [HACKERS] GUC for cleanup indexes threshold.

[Stephen Frost]

Greetings,

(pruned the CC list)

* Michael Paquier (michael.paqu...@gmail.com) wrote:
> On Thu, Nov 30, 2017 at 12:06 AM, Masahiko Sawada  
> wrote:
> > On Wed, Nov 29, 2017 at 11:05 PM, Simon Riggs  wrote:
> >> Unless there is disagreement on the above, it seems we should apply
> >> Yura's patch (an edited version, perhaps).
> >>
> >
> > IIRC the patches that makes the cleanup scan skip has a problem
> > pointed by Peter[1], that is that we stash an XID when a btree page is
> > deleted, which is used to determine when it's finally safe to recycle
> > the page. Yura's patch doesn't have that problem?
> >
> > [1] 
> > https://www.postgresql.org/message-id/CAH2-Wz%3D1%3Dt5fcGGfarQGcAWBqaCh%2BdLMjpYCYHpEyzK8Qg6OrQ%40mail.gmail.com
> 
> The latest patch present on this thread does not apply
> (https://www.postgresql.org/message-id/c4a47caef28024ab7528946bed264...@postgrespro.ru).
> Please send a rebase. For now I am moving the patch to next CF, with
> "waiting on author".

We are now just about a week into the CF and this patch hasn't been
updated and is still in 'waiting on author' state and it sounds like it
might have a pretty serious issue based on the above comment.

Masahiko Sawada, if this patch isn't viable or requires serious rework
to be acceptable, then perhaps we should change its status to 'returned
with feedback' and you can post a new patch for the next commitfest..?

Otherwise, I'd encourage you to try and find time to post an updated
patch soon, so that it can be reviewed.

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: [HACKERS] [PATCH] Vacuum: Update FSM more frequently

[Stephen Frost]

Greetings Claudio,

* Michael Paquier (michael.paqu...@gmail.com) wrote:
> On Mon, Nov 27, 2017 at 2:39 PM, Jing Wang  wrote:
> > A few general comments.
> >
> > +   FreeSpaceMapVacuum(onerel, 64);
> >
> > Just want to know why '64' is used here? It's better to give a description.
> >
> > +else
> > +   {
> > +   newslot = fsm_get_avail(page, 0);
> > +   }
> >
> > Since there is only one line in the else the bracket will not be needed. And
> > there in one more space ahead of else which should be removed.
> >
> >
> > +   if (nindexes == 0 &&
> > +   (vacuumed_pages_at_fsm_vac - vacuumed_pages) >
> > vacuum_fsm_every_pages)
> > +   {
> > +   /* Vacuum the Free Space Map */
> > +   FreeSpaceMapVacuum(onerel, 0);
> > +   vacuumed_pages_at_fsm_vac = vacuumed_pages;
> > +   }
> >
> > vacuumed_pages_at_fsm_vac is initialised with value zero and seems no chance
> > to go into the bracket.
> 
> The patch presented still applies, and there has been a review two
> days ago. This is a short delay to answer for the author, so I am
> moving this patch to next CF with the same status of waiting on
> author.

Looks like this patch probably still applies and the changes suggested
above seem pretty small, any chance you can provide an updated patch
soon Claudio, so that it can be reviewed properly during this
commitfest?

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: [HACKERS] Vacuum: allow usage of more than 1GB of work mem

[Stephen Frost]

Greetings,

* Michael Paquier (michael.paqu...@gmail.com) wrote:
> On Mon, Dec 4, 2017 at 2:38 PM, Claudio Freire  wrote:
> > They did apply at the time, but I think major work on vacuum was
> > pushed since then, and also I was traveling so out of reach.
> >
> > It may take some time to rebase them again. Should I move to needs
> > review myself after that?
> 
> Sure, if you can get into this state, please feel free to update the
> status of the patch yourself.

We're now over a month since this status update- Claudio, for this to
have a chance during this commitfest to be included (which, personally,
I think would be great as it solves a pretty serious issue..), we really
need to have it be rebased and updated.  Once that's done, as Michael
says, please change the patch status back to 'Needs Review'.

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: [HACKERS] Cached plans and statement generalization

[Stephen Frost]

Greetings,

* Konstantin Knizhnik (k.knizh...@postgrespro.ru) wrote:
> On 30.11.2017 04:59, Michael Paquier wrote:
> >On Wed, Sep 13, 2017 at 2:11 AM, Konstantin Knizhnik
> > wrote:
> >>One more patch passing all regression tests with autoprepare_threshold=1.
> >>I still do not think that it should be switch on by default...
> >This patch does not apply, and did not get any reviews. So I am moving
> >it to next CF with waiting on author as status. Please provide a
> >rebased version. Tsunakawa-san, you are listed as a reviewer of this
> >patch. If you are not planning to look at it anymore, you may want to
> >remove your name from the related CF entry
> >https://commitfest.postgresql.org/16/1150/.
> 
> Updated version of the patch is attached.

This patch appears to apply with just a bit of fuzz and make check
passes, so I'm not sure why this is currently marked as 'Waiting for
author'.

I've updated it to be 'Needs review'.  If that's incorrect, feel free to
change it back with an explanation.

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: [HACKERS] SQL/JSON in PostgreSQL

[Nikita Glukhov]

On 07.01.2018 00:11, Pavel Stehule wrote:

2018-01-06 22:02 GMT+01:00 Oleg Bartunov >:


On Sat, Jan 6, 2018 at 8:22 AM, Pavel Stehule
mailto:pavel.steh...@gmail.com>> wrote:
> Hi
>
> I am checking the JSONPath related code
>
> Questions, notes:
>
> 1. jsonpath operators are not consistent with any other .. json,
xml .. I am
> missing ?, @> operátors

I have slides about jsonpath
http://www.sai.msu.su/~megera/postgres/talks/sqljson-pgconf.eu-2017.pdf


> 2. documentation issue - there is "'{"a":[1,2,3,4,5]}'::json *?
'$.a[*] ? (@
>> 2)'" - operator *? doesn't exists

There are should be @? operator

> 3. operator @~ looks like too aggressive shortcut - should be better
> commented
>
> What is not clean, if jsonpath should to create some new
operators for json,
> jsonb types? It is special filter, defined by type, so from my
perspective
> the special operators are not necessary.

It's impossible to distinguish jsonpath from text, so introducing
new operators
are easier than everytime explicitly specify jsonpath datatype.


There are two possible solutions - special operator or explicit 
casting. In this case I am not sure if special operator for this case 
is good solution. Probably nobody will use it - because there SQL/JSON 
functions, but I don't think so this inconsistency is correct.


I have not strong opinion about it - it will be hidden feature for 
almost all users.



Operators are necessary for index support now.

Operators allows us to use a more concise syntax in simple cases, when 
we extract JSON item(s) without error handling:

js @* '$.key'
vs
JSON_QUERY(js, '$.key' RETURNING jsonb ERROR ON ERROR)


Also @* оperator gives us ability to extract a set of JSON items. 
JSON_QUERY can only wrap extracted item sequence into JSON array which 
we have to unwrap with our json[b]_array_elements() function. I also 
thought about returning setof-types in JSON_VALUE/JSON_QUERY:


JSON_QUERY(jsonb '[1,2,3]', '$[*]' RETURNING SETOF jsonb)

But it is not so easy to implement now, because we should introduce new 
node like TableFunc (or also we can try to use existing JSON_TABLE 
infrastructure).


Set-returning expressions are not allowed in every context, so for 
returning singleton items there should be additional operator.


--
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: [HACKERS] Surjective functional indexes

[Stephen Frost]

Greetings,

* Konstantin Knizhnik (k.knizh...@postgrespro.ru) wrote:
> On 15.12.2017 01:21, Michael Paquier wrote:
> >On Fri, Dec 15, 2017 at 6:15 AM, Alvaro Herrera  
> >wrote:
> >>Konstantin Knizhnik wrote:
> >>>If you still thing that additional 16 bytes per relation in statistic is 
> >>>too
> >>>high overhead, then I will also remove autotune.
> >>I think it's pretty clear that these additional bytes are excessive.
> >The bar to add new fields in PgStat_TableCounts in very high, and one
> >attempt to tackle its scaling problems with many relations is here by
> >Horiguchi-san:
> >https://www.postgresql.org/message-id/20171211.201523.24172046.horiguchi.kyot...@lab.ntt.co.jp
> >His patch may be worth a look if you need more fields for your
> >feature. So it seems to me that the patch as currently presented has
> >close to zero chance to be committed as long as you keep your changes
> >to pgstat.c.
> 
> Ok, looks like everybody think that autotune based on statistic is bad idea.
> Attached please find patch without autotune.

This patch appears to apply with a reasonable amount of fuzz, builds,
and passes 'make check', at least, therefore I'm going to mark it
'Needs Review'.

I will note that the documentation doesn't currently build due to this:

/home/sfrost/git/pg/dev/cleanup/doc/src/sgml/ref/create_index.sgml:302: parser 
error : Opening and ending tag mismatch: literal line 302 and unparseable
recheck_on_update

but I don't think it makes sense for that to stand in the way of someone
doing a review of the base patch.  Still, please do fix the
documentation build when you get a chance.

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: [HACKERS] Secondary index access optimizations

[Stephen Frost]

Greetings,

* Konstantin Knizhnik (k.knizh...@postgrespro.ru) wrote:
> On 04.12.2017 19:44, Alvaro Herrera wrote:
> >Konstantin Knizhnik wrote:
> >>
> >>On 30.11.2017 05:16, Michael Paquier wrote:
> >>>On Mon, Nov 6, 2017 at 10:13 PM, Konstantin Knizhnik
> >>> wrote:
> Concerning broken partition_join test: it is "expected" failure: my patch
> removes from the plans redundant checks.
> So the only required action is to update expected file with results.
> Attached please find updated patch.
> >>>The last patch version has conflicts in regression tests and did not
> >>>get any reviews. Please provide a rebased version. I am moving the
> >>>patch to next CF with waiting on author as status. Thanks!
> >>Rebased patch is attached.
> >I don't think this is a rebase on the previously posted patch ... it's
> >about 10x as big and appears to be a thorough rewrite of the entire
> >optimizer.
> 
> Or, sorry. I really occasionally committed in this branch patch for
> aggregate push down.
> Correct reabased patch is attached.

This patch applies, builds and passes 'make check-world', with no real
review posted of it, so I don't believe it should be 'Waiting on Author'
but really in 'Needs Review' status, so I've gone ahead and updated the
CF with that status.

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: [HACKERS] [PATCH] Overestimated filter cost and its mitigation

[Stephen Frost]

Greetings,

* Michael Paquier (michael.paqu...@gmail.com) wrote:
> On Thu, Nov 9, 2017 at 12:33 PM, Ashutosh Bapat
>  wrote:
> > Looking at order_qual_clauses(), we can say that a set of quals q1
> >  qn are ordered the same irrespective of the set of clauses they
> > are subset of. E.g. if {q1 .. qn} is subset of Q (ordered as Qo) and
> > also Q' (ordered as Q'o) the order in which they appear in Qo and Q'o
> > is same. So, even if different paths segregate the clauses in
> > different set, within each set the order is same. FWIW, we can order
> > all clauses in largest set once and use that order every time. Albeit
> > we will have to remember the order somewhere OR make the separator
> > routine retain the order in the larger set, which I guess is true
> > about all separator functions.
> 
> I am not sure what to think about this patch, so moved to next CF. The
> patch still applies. Hayamizu-san, it would be nice as well if you
> could review other's patches. One patch reviewed for one patch
> submitted, with equal difficulty. You should also get a community
> account so as it is possible to add your name as an author of this
> patch.

While this patch does still apply, it doesn't pass the 'make check'
regression tests and there appears to be concern raised up-thread about
the performance impact.

Yuto Hayamizu, it looks like some good questions were raised and which
need to be addressed, in addition to making sure that the patch at least
passes 'make check', so I'm leaving this as waiting-for-author.  If you
believe the patch is no longer viable, we can change it to 'returned
with feedback', otherwise, an updated patch and some responses to the
questions raised would be great as we're already a week into this
commitfest and this thread has been dormant since near the start of the
last CF.

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: [HACKERS] GUC for cleanup indexes threshold.

[Peter Geoghegan]

On Sat, Jan 6, 2018 at 2:20 PM, Stephen Frost  wrote:
>> > IIRC the patches that makes the cleanup scan skip has a problem
>> > pointed by Peter[1], that is that we stash an XID when a btree page is
>> > deleted, which is used to determine when it's finally safe to recycle
>> > the page. Yura's patch doesn't have that problem?
>> >
>> > [1] 
>> > https://www.postgresql.org/message-id/CAH2-Wz%3D1%3Dt5fcGGfarQGcAWBqaCh%2BdLMjpYCYHpEyzK8Qg6OrQ%40mail.gmail.com

> Masahiko Sawada, if this patch isn't viable or requires serious rework
> to be acceptable, then perhaps we should change its status to 'returned
> with feedback' and you can post a new patch for the next commitfest..?

I believe that the problem that I pointed out with freezing/wraparound
is a solvable problem. If we think about it carefully, we will come up
with a good solution. I have tried to get the ball rolling with my
pd_prune_xid suggestion. I think it's important to not lose sight of
the fact that the page deletion/recycling XID thing is just one detail
that we need to think about some more.

I cannot fault Sawada-san for waiting to hear other people's views
before proceeding. It really needs to be properly discussed.

-- 
Peter Geoghegan

Re: to_typemod(type_name) information function

[Tom Lane]

Sophie Herold  writes:
> the following patch allows to retrieve the typemod. Without this patch,
> it does not seem to be possible to generate the first column.

I thought about this a bit, and I now follow the problem you want to
solve, and agree that format_type() is going in the wrong direction.
However, the proposed solution seems a bit grotty.  You're basically
always going to need to run parseTypeString twice on the same input,
because there are few if any use-cases for getting just the typmod
without the type OID.  I think it might be more useful to provide
a single function

parse_type(type_name text, OUT typeid regtype, OUT typmod integer)

which would replace both to_regtype and to_typemod in your example.
Usage might look like

SELECT format_type(typeid, typmod)
  FROM (VALUES
('INTERVAL SECOND (5)'),
('Varchar(17)'),
('timestamptz (2)')) AS x(t), parse_type(x.t);

Creating a function with multiple OUT parameters at the pg_proc.h level
is slightly painful, but see e.g. pg_sequence_parameters for a model.

regards, tom lane

Re: [HACKERS] plpgsql - additional extra checks

[Stephen Frost]

Greetings Pavel,

* Pavel Stehule (pavel.steh...@gmail.com) wrote:
> 2017-11-30 3:44 GMT+01:00 Michael Paquier :
> > At least documentation needs patching, or this is going to generate
> > warnings on HEAD at compilation. I am moving this to next CF for lack
> > of reviews, and the status is waiting on author as this needs at least
> > a couple of doc fixes.
> 
> fixed doc attached

Looks like this patch should have been in "Needs Review" state, not
"Waiting for author", since it does apply, build and pass make
check-world, but as I'm signed up to review it, I'll do so here:

diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml
index 7d23ed437e..efa48bc13c 100644
--- a/doc/src/sgml/plpgsql.sgml
+++ b/doc/src/sgml/plpgsql.sgml
@@ -4963,6 +4963,11 @@ a_output := a_output || $$ if v_$$ || referrer_keys.kind 
|| $$ like '$$
 so you are advised to test in a separate development environment.

 
+   
+The setting plpgsql.extra_warnings to 
all is a 
+good idea in developer or test environments.
+   

Better language for this would be:

Setting plpgsql.extra_warnings, or
plpgsql.extra_errors, as appropriate, to
all is encouraged in development and/or testing
environments.

@@ -4979,6 +4984,30 @@ a_output := a_output || $$ if v_$$ || referrer_keys.kind 
|| $$ like '$$
  
 

+
+   
+strict_multi_assignment
+
+ 
+  Some PL/PgSQL commands allows to assign a 
values to
+  more than one variable. The number of target variables should not be
+  equal to number of source values. Missing values are replaced by NULL
+  value, spare values are ignored. More times this situation signalize
+  some error.
+ 
+
+   

Better language:

Some PL/PgSQL commands allow assigning values
to more than one variable at a time, such as SELECT INTO.  Typically,
the number of target variables and the number of source variables should
match, though PL/PgSQL will use NULL for
missing values and extra variables are ignored.  Enabling this check
will cause PL/PgSQL to throw a WARNING or
ERROR whenever the number of target variables and the number of source
variables are different.

+   
+too_many_rows
+
+ 
+  When result is assigned to a variable by INTO clause,
+  checks if query returns more than one row. In this case the assignment
+  is not deterministic usually - and it can be signal some issues in 
design.
+ 
+
+   
   

Better language:

Enabling this check will cause PL/PgSQL to
check if a given query returns more than one row when an
INTO clause is used.  As an INTO statement will only
ever use one row, having a query return multiple rows is generally
either inefficient and/or nondeterministic and therefore is likely an
error.

@@ -4997,6 +5026,34 @@ WARNING:  variable "f1" shadows a previously defined 
variable
 LINE 3: f1 int;
 ^
 CREATE FUNCTION
+
+
+  The another example shows the effect of 
plpgsql.extra_warnings
+  set to strict_multi_assignment:
+

Better language:

The below example shows the effects of setting
plpgsql.extra_warnings to
strict_multi_assignment:

diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index ec480cb0ba..0879e84cd2 100644
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -3629,6 +3629,24 @@ exec_stmt_execsql(PLpgSQL_execstate *estate,
longtcount;
int rc;
PLpgSQL_expr *expr = stmt->sqlstmt;
+   booltoo_many_rows_check;
+   int too_many_rows_level;
+
+   if (plpgsql_extra_errors & PLPGSQL_XCHECK_TOOMANYROWS)
+   {
+   too_many_rows_check = true;
+   too_many_rows_level = ERROR;
+   }
+   else if (plpgsql_extra_warnings & PLPGSQL_XCHECK_TOOMANYROWS)
+   {
+   too_many_rows_check = true;
+   too_many_rows_level = WARNING;
+   }
+   else
+   {
+   too_many_rows_check = false;
+   too_many_rows_level = NOTICE;
+   }


I'm not sure why we need two variables here- couldn't we simply look at
too_many_rows_level?  eg: too_many_rows_level >= WARNING ? ...

Not as big a deal, but I would change it to be 'check_too_many_rows' as
a variable name too.
 
@@ -3678,7 +3696,7 @@ exec_stmt_execsql(PLpgSQL_execstate *estate,
 */
if (stmt->into)
{
-   if (stmt->strict || stmt->mod_stmt)
+   if (stmt->strict || stmt->mod_stmt || too_many_rows_check)
tcount = 2;
else
tcount = 1;

The comment above this block needs updating for this change and, in
general, there's probably other pieces of code that this patch
changes where the comments should either be improved or ones added.


@@ -6033,12 +6051,48 @@ exec_move_row(PLpgSQL_execstate *estate,
int t_natts;
int fnum;
int 

Re: [HACKERS] GUC for cleanup indexes threshold.

[Stephen Frost]

Greetings Peter,

* Peter Geoghegan (p...@bowt.ie) wrote:
> On Sat, Jan 6, 2018 at 2:20 PM, Stephen Frost  wrote:
> >> > IIRC the patches that makes the cleanup scan skip has a problem
> >> > pointed by Peter[1], that is that we stash an XID when a btree page is
> >> > deleted, which is used to determine when it's finally safe to recycle
> >> > the page. Yura's patch doesn't have that problem?
> >> >
> >> > [1] 
> >> > https://www.postgresql.org/message-id/CAH2-Wz%3D1%3Dt5fcGGfarQGcAWBqaCh%2BdLMjpYCYHpEyzK8Qg6OrQ%40mail.gmail.com
> 
> > Masahiko Sawada, if this patch isn't viable or requires serious rework
> > to be acceptable, then perhaps we should change its status to 'returned
> > with feedback' and you can post a new patch for the next commitfest..?
> 
> I believe that the problem that I pointed out with freezing/wraparound
> is a solvable problem. If we think about it carefully, we will come up
> with a good solution. I have tried to get the ball rolling with my
> pd_prune_xid suggestion. I think it's important to not lose sight of
> the fact that the page deletion/recycling XID thing is just one detail
> that we need to think about some more.
> 
> I cannot fault Sawada-san for waiting to hear other people's views
> before proceeding. It really needs to be properly discussed.

Thanks for commenting!

Perhaps it should really be in Needs review state then..?

Either way, thanks again for the clarification and hopefully this will
revive the discussion.

Stephen


signature.asc
Description: PGP signature

Parallel append plan instability/randomness

[Tom Lane]

According to buildfarm member silverfish,

https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=silverfish&dt=2018-01-06%2008%3A53%3A38

it's possible to sometimes get this failure in the regression tests:

*** 
/mnt/buildfarm/buildroot/HEAD/pgsql.build/../pgsql/src/test/regress/expected/select_parallel.out
Tue Dec 19 20:24:02 2017
--- 
/mnt/buildfarm/buildroot/HEAD/pgsql.build/src/test/regress/results/select_parallel.out
  Sat Jan  6 09:21:39 2018
***
*** 75,84 
   Workers Planned: 3
   ->  Partial Aggregate
 ->  Parallel Append
   ->  Seq Scan on d_star
   ->  Seq Scan on f_star
   ->  Seq Scan on e_star
-  ->  Seq Scan on b_star
   ->  Seq Scan on c_star
   ->  Seq Scan on a_star
  (11 rows)
--- 75,84 
   Workers Planned: 3
   ->  Partial Aggregate
 ->  Parallel Append
+  ->  Seq Scan on b_star
   ->  Seq Scan on d_star
   ->  Seq Scan on f_star
   ->  Seq Scan on e_star
   ->  Seq Scan on c_star
   ->  Seq Scan on a_star
  (11 rows)

Irreproducible failures in the regression tests are not very acceptable.
Furthermore, considering that the query being tested is

explain (costs off)
  select round(avg(aa)), sum(aa) from a_star;

it seems to me that the "expected" order of the sub-scans is mighty
random to begin with.  A non-parallel implementation of the same
query will consistently scan these tables in their inheritance order:

 Aggregate
   ->  Append
 ->  Seq Scan on a_star
 ->  Seq Scan on b_star
 ->  Seq Scan on c_star
 ->  Seq Scan on d_star
 ->  Seq Scan on e_star
 ->  Seq Scan on f_star

Could we fix the parallel case to behave similarly please?

regards, tom lane

Re: [HACKERS] Refactoring identifier checks to consistently use strcmp

[Stephen Frost]

Greetings Michael, Daniel, all,

* Michael Paquier (michael.paqu...@gmail.com) wrote:
> On Fri, Dec 1, 2017 at 4:14 AM, Robert Haas  wrote:
> > I think the changes in DefineView and ATExecSetRelOptions is wrong,
> > because transformRelOptions() is still using pg_strcasecmp.  With the
> > patch:
> >
> > rhaas=# create view v(x) with ("Check_option"="local") as select 1;
> > CREATE VIEW
> > rhaas=# create view v(x) with ("check_option"="local") as select 1;
> > ERROR:  WITH CHECK OPTION is supported only on automatically updatable views
> > HINT:  Views that do not select from a single table or view are not
> > automatically updatable.
> >
> > Oops.
> 
> I am getting the feeling that there could be other issues than this
> one... If this patch ever gets integrated, I think that this should
> actually be shaped as two patches:
> - One patch with the set of DDL queries taking advantage of the
> current grammar with pg_strcasecmp.
> - A second patch that does the switch from pg_strcasecmp to strcmp,
> and allows checking which paths of patch 1 are getting changed.
> Patch 1 is the hard part to figure out all the possible patterns that
> could be changed.
> 
> > I am actually pretty dubious that we want to do this.  I found one bug
> > already (see above), and I don't think there's any guarantee that it's
> > the only one, because it's pretty hard to be sure that none of the
> > remaining calls to pg_strcasecmp are being applied to any of these
> > values in some other part of the code.  I'm not sure that the backward
> > compatibility issue is a huge deal, but from my point of view this
> > carries a significant risk of introducing new bugs, might annoy users
> > who spell any of these keywords in all caps with surrounding quotation
> > marks, and really has no clear benefit that I can see.  The
> > originally-offered justification is that making this consistent would
> > help us avoid subtle bugs, but it seems at least as likely to CREATE
> > subtle bugs, and the status quo is AFAICT harming nobody.
> 
> Changing opinion here ;)
> Yes, I agree that the risk of bugs may not be worth the compatibility
> effort at the end. I still see value in this patch for long-term
> purposes by making the code more consistent though.

Looks like this discussion has progressed to where this patch should
really be marked as Rejected.  Does anyone else want to voice an opinion
regarding it, or perhaps Daniel could post a rebuttal to the concerns
raised here?

Thinking through it, for my own 2c on this, I tend to agree with Tom
that, really, we should be using strcmp() for anything coming out of the
parser and that the backward-compatibility break from that is
acceptable.  I also understand Robert's concern that there may be other
bugs hiding and I wonder if there might be a way to check for them,
though no great ideas spring to mind offhand.  Would be great to hear
your thoughts, Daniel, so leaving this in Waiting on Author for now.

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: [HACKERS] Refactoring identifier checks to consistently use strcmp

[Tom Lane]

Stephen Frost  writes:
> Thinking through it, for my own 2c on this, I tend to agree with Tom
> that, really, we should be using strcmp() for anything coming out of the
> parser and that the backward-compatibility break from that is
> acceptable.  I also understand Robert's concern that there may be other
> bugs hiding and I wonder if there might be a way to check for them,
> though no great ideas spring to mind offhand.  Would be great to hear
> your thoughts, Daniel, so leaving this in Waiting on Author for now.

FWIW, I don't especially agree with Robert's position, because I think
he is ignoring the argument that it's a bug that some things are
case-sensitive and other seemingly similar things are not.

It's definitely concerning that the submitted patch introduced a new bug,
but we have seldom taken the position that bugs in an initial submission
are sufficient grounds for rejecting the entire concept.

ISTM that if this patch gets rid of a large fraction of the uses of
pg_strcasecmp in the backend, which is what I expect it should, then
it might not be out of reach to go through all the surviving ones to
make sure they are not processing strings that originate in the parser.

regards, tom lane

Re: proposal: alternative psql commands quit and exit

[Chapman Flack]

I'm not sure what the usual shape of 'consensus' or 'resolution' is in these 
things,
but it seems to me that the branch of this email thread that leads here 
(through the
message from Robert that I'm replying to) is the one that felt like it was 
converging.

I thought it was converging on the idea that

  ^[whitespace]*(quit|exit)[whitespace]*(;[whitespace]*)?$

would be treated specially when interactive, whether on a first or a 
continuation line,
and that the special treatment would be a helpful explanatory message to the
interactive user, while nevertheless appending the text to the query buffer.

One possibly not-yet-converged point was whether the special treatment should
be the same on a first line, or should just actually quit in that case.

None of this is what the currently latest patch does, though.

-Chap

Re: [HACKERS] GUC for cleanup indexes threshold.

[Peter Geoghegan]

Hi Stephen,

On Sat, Jan 6, 2018 at 4:02 PM, Stephen Frost  wrote:
> Perhaps it should really be in Needs review state then..?

Probably.

As I pointed out already some time ago, this RecentGlobalXmin
interlock stuff is our particular implementation of what Lanin &
Shasha call "The Drain Technique" within "2.5 Freeing Empty Nodes".
It's not easy to understand why this is necessary in general (you have
to understand the whole paper for that), but our implementation of the
drain technique is simple. I'm afraid that I made the problem sound
scarier than it actually is.

As Lanin & Shasha put it: "freeing empty nodes by the drain technique
is transparent to the rest of the algorithm and can be added by a
separate module". nbtree doesn't actually care very much about XIDs --
testing an XID against RecentGlobalXmin was just the most convenient
way of using L&S's technique to defer recycling until it is definitely
safe. We only need to make sure that _bt_page_recyclable() cannot
become confused by XID wraparound to fix this problem -- that's it.

-- 
Peter Geoghegan

Re: proposal: alternative psql commands quit and exit

[Tom Lane]

Chapman Flack  writes:
> I'm not sure what the usual shape of 'consensus' or 'resolution' is in these 
> things,
> but it seems to me that the branch of this email thread that leads here 
> (through the
> message from Robert that I'm replying to) is the one that felt like it was 
> converging.

> I thought it was converging on the idea that

>   ^[whitespace]*(quit|exit)[whitespace]*(;[whitespace]*)?$

> would be treated specially when interactive, whether on a first or a 
> continuation line,
> and that the special treatment would be a helpful explanatory message to the
> interactive user, while nevertheless appending the text to the query buffer.

That's what I thought, too.  I also think that we should recognize "help"
under exactly the same circumstances --- right now only a subset of what
you specify here will trigger the help response.

> One possibly not-yet-converged point was whether the special treatment should
> be the same on a first line, or should just actually quit in that case.

I can get on board with actually quitting if it's on the first line,
which is equivalent to the current behavior for "help".  That is,
all three would do-what-it-says-on-the-tin if entered when the query
buffer is empty, while they'd all emit an explanatory message (maybe
the same for all three, or maybe not) if entered when the query
buffer is not empty.

I doubt we have consensus on just what the explanatory message should
say, but maybe the author can give us a proposal to shoot at.

regards, tom lane

Re: Fix a Oracle-compatible instr function in the documentation

[Tom Lane]

Tatsuo Ishii  writes:
>> The documentation of PL/pgSQL provides sample codes of Oracle-compatible
>> instr functions. However, the behaviour is a little differet.
>> Oracle's instr raises an error when the forth argument value is less than
>> zero, but the sample code returns zero. This patch fixes this.

> Your patch fixes only instr(string varchar, string_to_search varchar, 
> beg_index integer).
> However in the doc there is another instr(string varchar, string_to_search 
> varchar, beg_index integer, occur_index integer).

> Shouldn't this be fixed as well?

Nagata-san's proposed addition makes no sense in the second instr()
implementation, which has no occur_index parameter; it only makes sense
in the third one.  I think the submitted patch is probably against some
old version of plpgsql.sgml and the line numbers in it are confusing
"patch" into thinking it should go into the second instr() implementation.

I checked with http://rextester.com/l/oracle_online_compiler
and verified that Oracle throws an error for zero/negative occur_index,
so the patch is definitely correct as far as it goes.  However, poking
at it a bit further, there is another problem: our sample code disagrees
with Oracle about what negative beg_index means.  For example, our code
produces

regression=# select instr('foo bar baz bar quux', 'bar', -6) ;
 instr 
---
13
(1 row)

regression=# select instr('foo bar baz bar quux', 'bar', -7) ;
 instr 
---
 5
(1 row)

whereas I get this with Oracle:

select instr('foo bar baz bar quux', 'bar', -6) from dual
13
select instr('foo bar baz bar quux', 'bar', -7) from dual
13
select instr('foo bar baz bar quux', 'bar', -8) from dual
13
select instr('foo bar baz bar quux', 'bar', -9) from dual
5

Evidently, they consider that negative beg_index indicates
the last place where the target substring can *begin*, whereas
our code thinks it is the last place where the target can *end*.

After a bit of fooling around with it, I produced code that agrees
with Oracle as far as I can tell, but it wouldn't be a bad idea
for someone else to test it some more.

I eliminated a couple of obvious ineffiencies while at it, notably
using position() for what could be a simple equality test in the
backwards-search loops.  I also changed the header comment, which
was both very incomplete and wrong in detail.

While the response to negative occur_index is probably not that
interesting in the field, the non-equivalent behavior for negative
beg_index is very definitely something that could bite people doing
Oracle translations.  So I agree we'd better back-patch this, and
maybe even call it out as a bug fix in the release notes.

regards, tom lane

diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml
index 7d23ed4..9d9e362 100644
*** a/doc/src/sgml/plpgsql.sgml
--- b/doc/src/sgml/plpgsql.sgml
*** $$ LANGUAGE plpgsql STRICT IMMUTABLE;
*** 5650,5668 
  
  --
  -- instr functions that mimic Oracle's counterpart
! -- Syntax: instr(string1, string2, [n], [m]) where [] denotes optional parameters.
  --
! -- Searches string1 beginning at the nth character for the mth occurrence
! -- of string2.  If n is negative, search backwards.  If m is not passed,
! -- assume 1 (search starts at first character).
  --
  
  CREATE FUNCTION instr(varchar, varchar) RETURNS integer AS $$
- DECLARE
- pos integer;
  BEGIN
! pos:= instr($1, $2, 1);
! RETURN pos;
  END;
  $$ LANGUAGE plpgsql STRICT IMMUTABLE;
  
--- 5650,5668 
  
  --
  -- instr functions that mimic Oracle's counterpart
! -- Syntax: instr(string1, string2 [, n [, m]]) where [] denotes optional parameters.
  --
! -- Search string1, beginning at the nth character, for the mth occurrence
! -- of string2.  If n is negative, search backwards, starting at the abs(n)'th
! -- character from the end of string1.
! -- If n is not passed, assume 1 (search starts at first character).
! -- If m is not passed, assume 1 (find first occurrence).
! -- Returns starting index of string2 in string1, or 0 if string2 is not found.
  --
  
  CREATE FUNCTION instr(varchar, varchar) RETURNS integer AS $$
  BEGIN
! RETURN instr($1, $2, 1);
  END;
  $$ LANGUAGE plpgsql STRICT IMMUTABLE;
  
*** BEGIN
*** 5688,5700 
  ELSIF beg_index < 0 THEN
  ss_length := char_length(string_to_search);
  length := char_length(string);
! beg := length + beg_index - ss_length + 2;
  
  WHILE beg > 0 LOOP
  temp_str := substring(string FROM beg FOR ss_length);
! pos := position(string_to_search IN temp_str);
! 
! IF pos > 0 THEN
  RETURN beg;
  END IF;
  
--- 5688,5698 
  ELSIF beg_index < 0 THEN
  ss_length := char_length(string_to_search);
  length := char_length(string);
! beg := length + 1 + beg_index;
  
  WHILE beg > 0 LOOP
  temp_str := substring(string FROM beg FOR s

Re: Add default role 'pg_access_server_files'

[Ryan Murphy]

Hi Stephen,

I have another question then based on what you said earlier today, and some 
testing I did using your patch.

TLDR: I created a role "tester" and was (as expected) not able to perform 
pg_read_file() on files outside the data directory.
But then I granted EXECUTE on that function for that role, and then I was able 
to (which is not what I expected).

Here's what I did (I apologize if this is too verbose):

* I successfully applied your patch to HEAD, and built Postgres from source:

make clean
configure (with options including a specific --prefix)
make
make install

* Then I went into the "install_dir/bin" and did the following to setup a data 
directory:

$ ./initdb ~/sql-data/2018-01-06
The files belonging to this database system will be owned by user 
"postgres".
This user must also own the server process.

The database cluster will be initialized with locale "en_US.UTF-8".
The default database encoding has accordingly been set to "UTF8".
The default text search configuration will be set to "english".

Data page checksums are disabled.

creating directory /Users/postgres/sql-data/2018-01-06 ... ok
creating subdirectories ... ok
selecting default max_connections ... 100
selecting default shared_buffers ... 128MB
selecting dynamic shared memory implementation ... posix
creating configuration files ... ok
running bootstrap script ... ok
performing post-bootstrap initialization ... ok
syncing data to disk ... ok

WARNING: enabling "trust" authentication for local connections
You can change this by editing pg_hba.conf or using the option -A, or
--auth-local and --auth-host, the next time you run initdb.

Success. You can now start the database server using:

./pg_ctl -D /Users/postgres/sql-data/2018-01-06 -l logfile start

* Then I started the database:

$ ./pg_ctl -D /Users/postgres/sql-data/2018-01-06 -l logfile start
waiting for server to start done
server started

* I went into the database and tried a pg_read_file:

$ psql postgres
psql (9.4.5, server 11devel)
Type "help" for help.

postgres=# select pg_read_file('/Users/postgres/temp');
   pg_read_file
---
 here is the file content
 
(1 row)

* Of course that worked as superuser, so created a new role:

postgres=# create role tester;
CREATE ROLE
postgres=# \q
postgres=# alter role tester with login;
ALTER ROLE
postgres=# \q

$ psql postgres tester
psql (9.4.5, server 11devel)
Type "help" for help.

postgres=> select pg_read_file('/Users/postgres/temp');
ERROR:  permission denied for function pg_read_file
postgres=> \q

* My current understanding at this point is that EXECUTE permissions would only 
allow "tester" to pg_read_file() on files in the data directory.  I try 
GRANTing EXECUTE:

$ psql postgres
psql (9.4.5, server 11devel)
Type "help" for help.

postgres=# grant execute on function pg_read_file(text) to tester;
GRANT
postgres=# select pg_read_file('/Users/postgres/temp');
   pg_read_file
---
 here is the file content
 
(1 row)



Is this expected behavior?  I thought I would need to GRANT that new 
"pg_access_server_files" role to "tester" in order to do this.  I may have 
misunderstood how your new feature works, just doublechecking.

Regards,
Ryan

Re: Add default role 'pg_access_server_files'

[Ryan Murphy]

Oops!  I made a mistake, which clearly showed up in my last email: I forgot
to psql back in as "tester".

Now I get the right behavior:

$ psql postgres tester
psql (9.4.5, server 11devel)
Type "help" for help.

postgres=> select pg_read_file('/Users/postgres/temp');
ERROR:  absolute path not allowed

Thanks for bearing with me.  So far so good on this feature, going to run
the tests too.

Best,
Ryan

Re: Add default role 'pg_access_server_files'

[Ryan Murphy]

(Duplicated to make sure it's in the commitfest Thread, didn't seem to get in 
there when I replied to the email)

Oops!  I made a mistake, which clearly showed up in my last email: I forgot to 
psql back in as "tester".

Now I get the right behavior:

$ psql postgres tester
postgres=> select pg_read_file('/Users/postgres/temp');
ERROR:  absolute path not allowed

Thanks for bearing with me.  So far so good on this feature, going to run the 
tests too.

Best,
Ryan

Re: Add default role 'pg_access_server_files'

[Ryan Murphy]

The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:   tested, passed
Spec compliant:   not tested
Documentation:tested, passed

I ran make installcheck-world and all tests passed except one that is a known 
issue with the way I have my environment setup (ecpg tests unrelated to this 
patch).

Manual tests I ran to see if it Implements the Feature:

1) confirmed that superuser can call pg_read_file() to read files in or out of 
data directory
2) confirmed that "tester" can call pg_read_file() only if given EXECUTE 
privilege
3) confirmed that "tester" can only call pg_read_file() on a file OUTSIDE of 
the data directory iff I "grant pg_access_server_files to tester;"

Documentation seems reasonable.

I believe this patch to be Ready for Committer.

The new status of this patch is: Ready for Committer