Re: TRUNCATE on foreign tables

[Michael Paquier]

On Thu, Jan 02, 2020 at 09:46:44AM -0500, Stephen Frost wrote:
> I agree that the FDW callback should support multiple tables in the
> TRUNCATE, but I think it also should include CASCADE as an option and
> have that be passed on to the FDW to handle.

As much as RESTRICT, ONLY and the INDENTITY clauses, no?  Just think
about postgres_fdw.
--
Michael


signature.asc
Description: PGP signature

Re: Patch to document base64 encoding

[Karl O. Pinc]

Hello Fabien,

On Sun, 5 Jan 2020 12:48:59 +0100 (CET)
Fabien COELHO  wrote:

> I'm in favor of moving and reorganizing these function descriptions,
> as they are somehow scattered with a unclear logic when you are
> looking for them.

I assume by this you mean you are happy with the organization done
by the patch.

For review (I think I've got this right) the organizational
changes are:

The changes suggested by Tom where 2
functions with the same name, one of which takes string 
arguments and the other of which takes bytea arguments
now show up both in the doc section on string functions and
in the doc section on bytea functions.

I believe I also alphabetized the binary function ordering.  

And this patch introduces a separate table/sub-section for 
functions which convert between binary and string.)
There are much-expanded descriptions of encode()
and decode().  (Which is how this patch series
started, explaining base64 encoding/decoding.)

FYI.  There is also an unusual usage of a hyperlinked
asterisk following the returned datatype of the hash
functions.  The hyperlink leads to the historical
note on the datatype used for the md5() function v.s.
the other hash functions.

>   +   bytea
> ||
>   +bytea
>bytea 
>   
>String concatenation
> 
> Bytea concatenation?

Done.  (Could just say "Concatenation" I suppose.  But "Bytea
concatenation" does not hurt and would be nice if you
ever looked at the tables for string operators and bytea
operators side-by-side.)

> I'm not keen on calling the parameter the name of its type. I'd
> suggest to keep "string" as a name everywhere, which is not a type
> name in Pg.
> 
> The functions descriptions are not homogeneous. Some have parameter
> name & type "btrim(string bytea, bytes bytea)" and others only type
> or parameter with tagged as a parameter "get_bit(bytea,
> offset)" (first param), "sha224(bytea)".
> 
> I'd suggest to be consistent, eg use "string bytea" everywhere 
> appropriate.

Ok.  Done.  Except that I've left the encode() function
as encode(data bytea, format text) because the whole
point is to convert _to_ a string/text datatype
from something that's _not_ a string.  Calling
the input a string just seems wrong.  This inconsistency seems ok
because encode() is in the table of string <-> bytea functions,
away from the other bytea functions.


If you're interested, another possibility would be the
consistent use of "data bytea" everywhere.  I like this
choice because it works well to write
encode(data bytea,
   formatstring bytea,
bytes bytea)

Remove the longest string containing only bytes appearing in
bytes from the start and end of
string


Would change to (say):

  btrim(data bytea,
bytes bytea)

  Remove the longest contiguous sequence of bytes containing only
  those bytes appearing in bytes
  from the start and end of data

The trouble with using "data bytea" is that there might
need to be adjustments to the word "string" elsewhere in
the section, not just in the descriptions.

Let me know if you'd prefer "data bytea" to "string bytea"
and consequent frobbing of descriptions.  That might be
out-of-scope for this patch.  (Which is already
a poster-child for feature-creep.)

Attached is doc_base64_v12.patch.

Regards,

Karl 
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 57a1539506..2e45bf71c8 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -1431,6 +1431,13 @@
 natively for the bit-string types.

 
+   
+ Functions which convert, both to and from, strings and
+ the bytea type
+ are documented
+ separately.
+   
+

 SQL defines some string functions that use
 key words, rather than commas, to separate
@@ -1792,101 +1799,6 @@
abcde,2,22
   
 
-  
-   
-
- convert
-
-convert(string bytea,
-src_encoding name,
-dest_encoding name)
-   
-   bytea
-   
-Convert string to dest_encoding.  The
-original encoding is specified by
-src_encoding. The
-string must be valid in this encoding.
-Conversions can be defined by CREATE CONVERSION.
-Also there are some predefined conversions. See  for available conversions.
-   
-   convert('text_in_utf8', 'UTF8', 'LATIN1')
-   text_in_utf8 represented in Latin-1
-   encoding (ISO 8859-1)
-  
-
-  
-   
-
- convert_from
-
-convert_from(string bytea,
-src_encoding name)
-   
-   text
-   
-Convert string to the database encoding.  The original encoding
-is specified by src_encoding. The
-string must be valid in this encoding.
-   
-   convert_from('text_in_utf8', 'UTF8')
-   text_in_utf8 represented in the current database encoding
-  

Re: error context for vacuum to include block number (atomic progress update)

[Michael Paquier]

On Sun, Dec 29, 2019 at 02:17:47PM -0600, Justin Pryzby wrote:
> The behavior is different from before, but I think that's ok: the number of
> scans is accurate, and the PHASE is accurate, even though it'll change a 
> moment
> later.

pgstat_progress_update_multi_param() is useful when it comes to update
multiple parameters at the same time consistently in a given progress
phase.  For example, in vacuum, when beginning the heap scan, the
number of blocks to scan and the max number of dead tuples has to be
updated at the same as the phase name, as things have to be reported
consistently, so that's critical to be consistent IMO.  Now, in this
case, we are discussing about updating a parameter which is related to
the index vacuuming phase, while switching at the same time to a
different phase.  I think that splitting both is not confusing here
because the number of times vacuum indexes have been done is unrelated
to the heap cleanup happening afterwards.  On top of that the new code
is more readable, and future callers of lazy_vacuum_heap() will never
miss to update the progress reporting to the new phase.

While on it, a "git grep -n" is showing me two places where we could
care more about being consistent by using the multi-param version of
progress reports when beginning a new progress phase:
- reindex_index()
- ReindexRelationConcurrently()

One can also note the switch to PROGRESS_VACUUM_PHASE_INDEX_CLEANUP in 
lazy_scan_heap() but it can be discarded for the same reason as what
has been refactored recently with the index vacuuming. 
--
Michael


signature.asc
Description: PGP signature

Re: color by default

[Gavin Flower]

On 06/01/2020 18:38, Michael Paquier wrote:

On Fri, Jan 03, 2020 at 01:10:30PM -0500, Robert Haas wrote:

On Thu, Jan 2, 2020 at 6:38 PM Gavin Flower
 wrote:

I find coloured output very difficult to read, as the colours seem to be
chosen on the basis everyone uses white as the background colour for
terminals -- whereas I use black, as do a lot of other people.

I don't like colored output either.

I don't like colored output either.  However there is an easy way to
disable that so applying this patch does not change things IMO as
anybody unhappy with colors can just disable it with a one-liner in
a bashrc or such.
--
Michael


That's kind of like using a sledgehammer to crack a nut.

The colour in grep output is often useful.

I'd like to control it per application.


Cheers,
Gavin

Re: Add pg_file_sync() to adminpack

[Michael Paquier]

On Mon, Jan 06, 2020 at 03:20:13PM +0900, Arthur Zakirov wrote:
> It isn't case if a file doesn't exist. But if there are no permissions on
> the file:
> 
> PANIC:  could not open file "testfile": Permissions denied
> server closed the connection unexpectedly
> 
> It could be fixed by implementing a function like pg_file_sync_internal() or
> by making the function fsync_fname_ext() external.

The patch uses stat() to make sure that the file exists and has no
issues.  Though it could be a problem with any kind of TOCTOU-like
issues (looking at you, Windows, for ENOPERM), so I agree that it
would make more sense to use pg_fsync() here with a fd opened first.
--
Michael


signature.asc
Description: PGP signature

Re: remove some STATUS_* symbols

[Michael Paquier]

On Sun, Dec 29, 2019 at 11:33:34AM +0100, Peter Eisentraut wrote:
> Attached are two patches to remove these two symbols.  STATUS_FOUND can be
> replaced by a simple bool.  STATUS_WAITING is replaced by a separate enum.

Patch 0001 looks good to me, but I got to wonder why the check after
waitMask in LockAcquireExtended() is not done directly in
LockCheckConflicts().

Regarding patch 0002, I am not sure that the addition of
ProcWaitStatus brings much though in terms of code readability.
--
Michael


signature.asc
Description: PGP signature

Re: parallel vacuum options/syntax

[Masahiko Sawada]

On Sun, 5 Jan 2020 at 23:28, Amit Kapila  wrote:
>
> On Sun, Jan 5, 2020 at 7:38 PM Masahiko Sawada
>  wrote:
> >
> > On Sun, 5 Jan 2020 at 22:39, Tomas Vondra  
> > wrote:
> > >
> > >
> > > So if we think we need an option to determine vacuum parallel degree, we
> > > should have an option to disable parallelism too. I don't care much if
> > > it's called DISABLE_PARALLEL, NOPARALLEL or PARALLEL 0, as long as we
> > > make our mind and don't unnecessarily break it in the next release.
> > >
>
> Fair point.  I favor parallel 0 as that avoids adding more options and
> also it is not very clear whether that is required at all.  Till now,
> if I see most people who have shared their opinion seems to favor this
> as compared to another idea where we need to introduce more options.
>
> >
> > Okay I got your point. It's just an idea but how about controlling
> > parallel vacuum using two options. That is, we have PARALLEL option
> > that takes a boolean value (true by default) and enables/disables
> > parallel vacuum. And we have WORKERS option that takes an integer more
> > than 1 to specify the number of workers. Of course we should raise an
> > error if only WORKERS option is specified. WORKERS option is optional.
> > If WORKERS option is omitted the number of workers is determined based
> > on the number of indexes on the table.
> >
>
> I think this would add failure modes without serving any additional
> purpose.  Sure, it might give the better feeling that we have separate
> options to enable/disable parallelism and then specify the number of
> workers with a separate option, but we already have various examples
> as shared by me previously where setting the value as zero means the
> option is disabled, so why to invent something new here?

I just felt it's not intuitive that specifying parallel degree to 0
means to disable parallel vacuum. But since majority of hackers seem
to agree with this syntax I'm not going to insist on that any further.

Regards,

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

Re: Add pg_file_sync() to adminpack

[Arthur Zakirov]

Hello,

On 2019/12/25 23:12, Julien Rouhaud wrote:

On Wed, Dec 25, 2019 at 2:01 PM Fujii Masao  wrote:


Hi,

I'd like to propose to add pg_file_sync() function into contrib/adminpack.
This function fsyncs the specified file or directory named by its argument.
IMO this is useful, for example, when you want to fsync the file that
pg_file_write() writes out or that COPY TO exports the data into,
for durability. Thought?


+1 too. I have a thought, but maybe it is just a nitpicking.

pg_file_sync() calls fsync_fname() function from fd.c. And I think it 
might bring problems because fsync_fname() uses data_sync_elevel() to 
get elevel. As a result if data_sync_retry GUC is false fsync_fname() 
might raise PANIC message.


It isn't case if a file doesn't exist. But if there are no permissions 
on the file:


PANIC:  could not open file "testfile": Permissions denied
server closed the connection unexpectedly

It could be fixed by implementing a function like 
pg_file_sync_internal() or by making the function fsync_fname_ext() 
external.



+1, that seems like a useful wrapper.  Looking at existing functions,
I see that there's a pg_file_rename() in adminpack, but it doesn't use
durable_rename nor does it try to perform any fsync.  Same for
pg_file_unlink vs. durable_unlink.  It's probably worth fixing that at
the same time?


I think it might be a different patch.

--
Arthur

Re: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes

[Tom Lane]

I wrote:
> Peter Eisentraut  writes:
>> I haven't found an explanation in this thread why it does always quote 
>> now.  That seems a bit unusual.  Is there a reason for this?  Can we do 
>> without it?

> The core problem we're trying to solve is stated in the thread title:
> ...
> It'd be possible, perhaps, to distinguish between this case and the
> cases in backslash commands, which are okay with omitted quotes
> (for some filenames).  I'm not sure that that would be an improvement
> though.

I realized that there *is* a good reason to worry about this.  Fairly
recent versions of libedit will try to backslash-escape the output
of psql_completion(), as I described over at [1].  This is absolutely
disastrous if we've emitted a quoted filename: we end up going from,
say,
\lo_import myf
to
\lo_import \'myfile\'
which of course doesn't work at all (psql thinks \' is in itself a
backslash command).

There isn't much we can do about this in contexts where we must
quote the filename: not doing so produces an equally broken command.
However, this problem does suggest that we shouldn't force quoting
if we don't have to, as that just breaks cases we needn't break.

Hence, the attached revision only forces quoting in a SQL COPY
command, or if the user already typed a quote.

I also added some regression tests (whee!).  They pass for me with a
couple different readline and libedit versions, but I have only minimal
hopes for them passing everywhere without further hacking ... the
results of the other thread suggest that pain is to be expected here.

regards, tom lane

[1] https://www.postgresql.org/message-id/13708.1578059577%40sss.pgh.pa.us

diff --git a/config/programs.m4 b/config/programs.m4
index 90ff944..68ab823 100644
--- a/config/programs.m4
+++ b/config/programs.m4
@@ -209,17 +209,20 @@ fi
 
 
 
-# PGAC_VAR_RL_COMPLETION_APPEND_CHARACTER
-# ---
+# PGAC_READLINE_VARIABLES
+# ---
 # Readline versions < 2.1 don't have rl_completion_append_character
+# Libedit lacks rl_filename_quote_characters and rl_filename_quoting_function
 
-AC_DEFUN([PGAC_VAR_RL_COMPLETION_APPEND_CHARACTER],
+AC_DEFUN([PGAC_READLINE_VARIABLES],
 [AC_CACHE_CHECK([for rl_completion_append_character], pgac_cv_var_rl_completion_append_character,
 [AC_LINK_IFELSE([AC_LANG_PROGRAM([#include 
-#ifdef HAVE_READLINE_READLINE_H
-# include 
+#if defined(HAVE_READLINE_READLINE_H)
+#include 
+#elif defined(HAVE_EDITLINE_READLINE_H)
+#include 
 #elif defined(HAVE_READLINE_H)
-# include 
+#include 
 #endif
 ],
 [rl_completion_append_character = 'x';])],
@@ -228,7 +231,42 @@ AC_DEFUN([PGAC_VAR_RL_COMPLETION_APPEND_CHARACTER],
 if test x"$pgac_cv_var_rl_completion_append_character" = x"yes"; then
 AC_DEFINE(HAVE_RL_COMPLETION_APPEND_CHARACTER, 1,
   [Define to 1 if you have the global variable 'rl_completion_append_character'.])
-fi])# PGAC_VAR_RL_COMPLETION_APPEND_CHARACTER
+fi
+AC_CACHE_CHECK([for rl_filename_quote_characters], pgac_cv_var_rl_filename_quote_characters,
+[AC_LINK_IFELSE([AC_LANG_PROGRAM([#include 
+#if defined(HAVE_READLINE_READLINE_H)
+#include 
+#elif defined(HAVE_EDITLINE_READLINE_H)
+#include 
+#elif defined(HAVE_READLINE_H)
+#include 
+#endif
+],
+[rl_filename_quote_characters = "x";])],
+[pgac_cv_var_rl_filename_quote_characters=yes],
+[pgac_cv_var_rl_filename_quote_characters=no])])
+if test x"$pgac_cv_var_rl_filename_quote_characters" = x"yes"; then
+AC_DEFINE(HAVE_RL_FILENAME_QUOTE_CHARACTERS, 1,
+  [Define to 1 if you have the global variable 'rl_filename_quote_characters'.])
+fi
+AC_CACHE_CHECK([for rl_filename_quoting_function], pgac_cv_var_rl_filename_quoting_function,
+[AC_LINK_IFELSE([AC_LANG_PROGRAM([#include 
+#if defined(HAVE_READLINE_READLINE_H)
+#include 
+#elif defined(HAVE_EDITLINE_READLINE_H)
+#include 
+#elif defined(HAVE_READLINE_H)
+#include 
+#endif
+],
+[rl_filename_quoting_function = 0;])],
+[pgac_cv_var_rl_filename_quoting_function=yes],
+[pgac_cv_var_rl_filename_quoting_function=no])])
+if test x"$pgac_cv_var_rl_filename_quoting_function" = x"yes"; then
+AC_DEFINE(HAVE_RL_FILENAME_QUOTING_FUNCTION, 1,
+  [Define to 1 if you have the global variable 'rl_filename_quoting_function'.])
+fi
+])# PGAC_READLINE_VARIABLES
 
 
 
diff --git a/configure b/configure
index d2d63f2..40fe9c1 100755
--- a/configure
+++ b/configure
@@ -16316,10 +16316,12 @@ else
   cat confdefs.h - <<_ACEOF >conftest.$ac_ext
 /* end confdefs.h.  */
 #include 
-#ifdef HAVE_READLINE_READLINE_H
-# include 
+#if defined(HAVE_READLINE_READLINE_H)
+#include 
+#elif defined(HAVE_EDITLINE_READLINE_H)
+#include 
 #elif defined(HAVE_READLINE_H)
-# include 
+#include 
 #endif
 
 int
@@ -16345,6 +16347,85 @@ if test x"$pgac_cv_var_rl_completion_append_character" = x"yes"; then
 $as_echo "#define HAVE_RL_COMPLETION_APPEND_CHARACTER 1" >>confdefs.h
 
 fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: checking for rl_filename_quote_characters" 

Re: Setting min/max TLS protocol in clientside libpq

[Michael Paquier]

On Thu, Jan 02, 2020 at 09:46:44PM +, cary huang wrote:
> I agree with Arthur that it makes sense to check the validity of
> "conn->sslmaxprotocolversion" first before checking if it is larger
> than "conn->sslminprotocolversion"

Here I don't agree.  Why not just let OpenSSL handle things with
SSL_CTX_set_min_proto_version?  We don't bother about that in the
backend code for that reason on top of keeping the code more simple
with less error handling.  And things are cleaner when it comes to
this libpq patch by giving up with the INT_MIN hack.

> A small suggestion here. I see that PG server defaults TLS min
> version to be TLSv1.2 and max version to none. So by default the
> server accepts TLSv1.2 and above. I think on the client side, it
> also makes sense to have the same defaults as the server.

Yeah, that makes sense.  Even more now that I have just removed
support for OpenSSL 0.9.8 and 1.0.0 ;)

There could be an argument to lower down the default if we count for
backends built with OpenSSL versions older than libpq, but I am not
ready to buy that there would be many of those.

> In the patch, if the client does not supply "sslminprotocolversion",
> it will run to "else" statement and sets TLS min version to "INT_MIN",
> which is a huge negative number and of course openssl won't set
> it. I think this else statement can be enhanced a little to set
> "sslminprotocolversion" to TLSv1.2 by default to match the server
> and provide some warning message that TLS minimum has defaulted to
> TLSv1.2. 

In this patch fe-secure-openssl.c has just done a copy-paste of
SSL_CTX_set_min_proto_version and SSL_CTX_set_max_proto_version
present in be-secure-openssl.c.  That's not good.  Could you refactor
that please as a separate file?  For example openssl-protocol.c in
src/common/?  src/common/ stuff is built with -fPIC since 7143b3e so
there is no need to include directly the source files in the
Makefile.  A shame you cannot do that for
ssl_protocol_version_to_openssl(), so for that a note would be welcome
on top of the former backend routine and the one you are adding.

The patch has conflicts with libpq-int.h as far as I can see.  That
should be easy enough to solve.

The patch should have tests in src/test/ssl/, like for invalid values,
incorrect combinations leading to failures, etc.
--
Michael


signature.asc
Description: PGP signature

Re: color by default

[Michael Paquier]

On Fri, Jan 03, 2020 at 01:10:30PM -0500, Robert Haas wrote:
> On Thu, Jan 2, 2020 at 6:38 PM Gavin Flower
>  wrote:
>> I find coloured output very difficult to read, as the colours seem to be
>> chosen on the basis everyone uses white as the background colour for
>> terminals -- whereas I use black, as do a lot of other people.
> 
> I don't like colored output either.

I don't like colored output either.  However there is an easy way to
disable that so applying this patch does not change things IMO as
anybody unhappy with colors can just disable it with a one-liner in
a bashrc or such.
--
Michael


signature.asc
Description: PGP signature

Re: [Proposal] Global temporary tables

[曾文旌(义从)]

In the previous communication

1 we agreed on the general direction
1.1 gtt use local (private) buffer
1.2 no replica access in first version

2 We feel that gtt needs to maintain statistics, but there is no agreement on 
what it will be done.

3 Still no one commented on GTT's transaction information processing, they 
include
3.1 Should gtt's frozenxid need to be care?
3.2 gtt’s clog clean
3.3 How to deal with "too old" gtt data

I suggest we discuss further, reach an agreement, and merge the two patches to 
one.


Wenjing


> 2020年1月6日 上午4:06，Tomas Vondra  写道：
> 
> Hi,
> 
> I think we need to do something with having two patches aiming to add
> global temporary tables:
> 
> [1] https://commitfest.postgresql.org/26/2349/
> 
> [2] https://commitfest.postgresql.org/26/2233/
> 
> As a reviewer I have no idea which of the threads to look at - certainly
> not without reading both threads, which I doubt anyone will really do.
> The reviews and discussions are somewhat intermixed between those two
> threads, which makes it even more confusing.
> 
> I think we should agree on a minimal patch combining the necessary/good
> bits from the various patches, and terminate one of the threads (i.e.
> mark it as rejected or RWF). And we need to do that now, otherwise
> there's about 0% chance of getting this into v13.
> 
> In general, I agree with the sentiment Rober expressed in [1] - the
> patch needs to be as small as possible, not adding "nice to have"
> features (like support for parallel queries - I very much doubt just
> using shared instead of local buffers is enough to make it work.)
> 
> regards
> 
> -- 
> Tomas Vondra  http://www.2ndQuadrant.com
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: doc: alter table references bogus table-specific planner parameters

[Simon Riggs]

On Mon, 6 Jan 2020 at 04:13, Justin Pryzby  wrote:

>
> > I agree with the sentiment of the third doc change, but your patch
> removes
> > the mention of n_distinct, which isn't appropriate.
>
> I think it's correct to remove n_distinct there, as it's documented
> previously,
> since e5550d5f.  That's a per-attribute option (not storage) and can't be
> specified there.
>

OK, then agreed.

> The second change in your patch alters the meaning of the sentence in a
> way
> > that is counter to the first change. The name of these parameters is
> > "Storage Parameters" (in various places); I might agree with describing
> > them in text as "storage or planner parameters", but if you do that you
> > can't then just refer to "storage parameters" later, because if you do it
> > implies that planner parameters operate differently to storage
> parameters,
> > which they don't.
>
> The 2nd change is:
>
>for details on the available parameters.  Note that the table
> contents
> -  will not be modified immediately by this command; depending on the
> +  will not be modified immediately by setting its storage parameters;
> depending on the
>parameter you might need to rewrite the table to get the desired
> effects.
>
> I deliberately qualified that as referring only to "storage params" rather
> than
> "this command", since planner params never "modify the table contents".
> Possibly other instances in the document (and createtable) should be
> changed
> for consistency.
>

Yes, but it's not a correction, just a different preference of wording.

-- 
Simon Riggshttp://www.2ndQuadrant.com/

PostgreSQL Solutions for the Enterprise

Re: doc: alter table references bogus table-specific planner parameters

[Justin Pryzby]

On Mon, Jan 06, 2020 at 03:48:52AM +, Simon Riggs wrote:
> On Mon, 6 Jan 2020 at 02:56, Justin Pryzby  wrote:
> 
> > commit 6f3a13ff058f15d565a30c16c0c2cb14cc994e42 Enhance docs for ALTER 
> > TABLE lock levels of storage parms
> > Author: Simon Riggs 
> > Date:   Mon Mar 6 16:48:12 2017 +0530
> >
> > 
> >  SET (  > class="PARAMETER">storage_parameter =  > class="PARAMETER">value [, ... ] )
> > ...
> > -  Changing fillfactor and autovacuum storage parameters acquires a
> > SHARE UPDATE EXCLUSIVE lock.
> > +  SHARE UPDATE EXCLUSIVE lock will be taken for
> > +  fillfactor and autovacuum storage parameters, as well as the
> > +  following planner related parameters:
> > +  effective_io_concurrency, parallel_workers, seq_page_cost
> > +  random_page_cost, n_distinct and n_distinct_inherited.
> >
> > effective_io_concurrency, seq_page_cost and random_page_cost cannot be set
> > for
> > a table - reloptions.c shows that they've always been
> > RELOPT_KIND_TABLESPACE.
> 
> I agree with the sentiment of the third doc change, but your patch removes
> the mention of n_distinct, which isn't appropriate.

I think it's correct to remove n_distinct there, as it's documented previously,
since e5550d5f.  That's a per-attribute option (not storage) and can't be
specified there.


 SET ( attribute_option = value [, ... ] )
 RESET ( attribute_option [, ... ] )
 
  
   This form sets or resets per-attribute options.  Currently, the only
...
+ 
+  Changing per-attribute options acquires a
+  SHARE UPDATE EXCLUSIVE lock.
+ 

> The second change in your patch alters the meaning of the sentence in a way
> that is counter to the first change. The name of these parameters is
> "Storage Parameters" (in various places); I might agree with describing
> them in text as "storage or planner parameters", but if you do that you
> can't then just refer to "storage parameters" later, because if you do it
> implies that planner parameters operate differently to storage parameters,
> which they don't.

The 2nd change is:

   for details on the available parameters.  Note that the table contents
-  will not be modified immediately by this command; depending on the
+  will not be modified immediately by setting its storage parameters; 
depending on the
   parameter you might need to rewrite the table to get the desired effects.

I deliberately qualified that as referring only to "storage params" rather than
"this command", since planner params never "modify the table contents".
Possibly other instances in the document (and createtable) should be changed
for consistency.

Justin

Re: [PATCH] Increase the maximum value track_activity_query_size

[Michael Paquier]

On Fri, Jan 03, 2020 at 01:48:56PM -0500, Tom Lane wrote:
> Robert Haas  writes:
>> I vote for not trying to make this more complicated and just accepting
>> the original proposal. It's about a factor of ten increase over the
>> limit we have right now, which doesn't seem like enough to cause any
>> real breakage, and it should be enough to satisfy the majority of the
>> people who are unhappy with the current limit, and it is very little
>> work.
> 
> +1 ... we've surely beaten this topic to death by now.

Sounds like an agreement then.  The original patch documents the range
in postgresql.conf.sample, which is fine by me as this is done for
some parameters, and skips the part about doc/, which also matches
with the surrounding effort for other parameters, so the whole looks
fine seen from here.  Anybody willing to commit that?
--
Michael


signature.asc
Description: PGP signature

Re:could not access status of transaction

[chenhj]

Hi,



This database has not had the same failure again 2019/09/16(reported at 
2019/09/29), so this is a very low probability failure, but it is uncertain 
whether it will happen again in the future. Now add some information for 
incident at 2019/09/16, may be useful for analyze the cause of this problem.




> Block 32291 
>  -
>  Block Offset: 0x0fc46000 Offsets: Lower 236 (0x00ec)
>  Block: Size 8192  Version4Upper2112 (0x0840)
>  LSN:  logid254 recoff 0xa3a598c8  Special  8192 (0x2000)
>  Items:   53  Free Space: 1876
>  Checksum: 0x8355  Prune XID: 0x02186566  Flags: 0x0001 (HAS_FREE_LINES)
>  Length (including item array): 236
> ...
>  Item   8 -- Length:  151  Offset: 4696 (0x1258)  Flags: NORMAL
>   XMIN: 35153480  XMAX: 35153545  CID|XVAC: 0
>   Block Id: 163363  linp Index: 9   Attributes: 20   Size: 32
>   infomask: 0x2503 
> (HASNULL|HASVARWIDTH|XMIN_COMMITTED|XMAX_COMMITTED|UPDATED|HOT_UPDATED|HEAP_ONLY)
>   t_bits: [0]: 0xff [1]: 0xff [2]: 0x07
> 
> 
> 
>  Item   9 -- Length:  151  Offset: 4544 (0x11c0)  Flags: NORMAL
>   XMIN: 35153545  XMAX: 0  CID|XVAC: 0
>   Block Id: 163363  linp Index: 9   Attributes: 20   Size: 32
>   infomask: 0x2803 (HASNULL|HASVARWIDTH|XMAX_INVALID|UPDATED|HEAP_ONLY)
>   t_bits: [0]: 0xff [1]: 0xff [2]: 0x07


According to above information, the flags of the heap page (163363) with the 
problem tuple (163363, 9) is 0x0001 (HAS_FREE_LINES), that is, ALL_VISIBLE is 
not set.


However, according  hexdump content of the corresponding vm file, that 
block(location is 9F88 + 6bit) has set VISIBILITYMAP_ALL_FROZEN and 
VISIBILITYMAP_ALL_VISIBLE flags. That is, the heap file and the vm file are 
inconsistent.


# vi 19548_vm.hexdump


000 0215  0858 857c 8cee  0018 2000
010 2000 2004      
020        
*
0002000 0215  1680 857c 3bb1  0018 2000
0002010 2000 2004      
0002020        
*
0004000 0215  20e8 857c 07f7  0018 2000
0004010 2000 2004      
0004020        
*
0006000 0215  3618 857c 4792  0018 2000
0006010 2000 2004      
0006020        
*
0008000 0215  17a8 8580 3d7e  0018 2000
0008010 2000 2004      
0008020        
*
000a000 0215  3558 8585 1239  0018 2000
000a010 2000 2004      
000a020        
*
000c000 0215  72a8 871b 1a23  0018 2000
000c010 2000 2004      
000c020        
...
000e000 0215  93d0 8794 506a  0018 2000
000e010 2000 2004   fc03   
000e020  f3ff cccf fffc   f3cf ff3f
...
000f6c0 3f0f 3303 c33f 00f0 00c3 0303 0003 
000f6d0        
*
001

Regards,
Chen Huajun

Re: Removal of support for OpenSSL 0.9.8 and 1.0.0

[Michael Paquier]

On Fri, Jan 03, 2020 at 10:57:54PM +0100, Daniel Gustafsson wrote:
> LGTM, switching to ready for committer.

Thanks Daniel.  I have looked at that stuff again, and committed the
patch.
--
Michael


signature.asc
Description: PGP signature

Re: PATCH: logical_work_mem and logical streaming of large in-progress transactions

[Dilip Kumar]

On Sat, Jan 4, 2020 at 4:07 PM Amit Kapila  wrote:
>
> On Mon, Dec 30, 2019 at 3:11 PM Dilip Kumar  wrote:
> >
> > On Thu, Dec 12, 2019 at 9:44 AM Dilip Kumar  wrote:
> > >
> > Yesterday, Tomas has posted the latest version of the patch set which
> > contain the fix for schema send part.  Meanwhile, I was working on few
> > review comments/bugfixes and refactoring.  I have tried to merge those
> > changes with the latest patch set except the refactoring related to
> > "0006-Implement-streaming-mode-in-ReorderBuffer" patch, because Tomas
> > has also made some changes in the same patch.
> >
>
> I don't see any changes by Tomas in that particular patch, am I
> missing something?
He has created some sub-patch from the main patch for handling
schema-sent issue.  So if I make change in that patch all other
patches will conflict.

>
> >  I have created a
> > separate patch for the same so that we can review the changes and then
> > we can merge them to the main patch.
> >
>
> It is better to merge it with the main patch for
> "Implement-streaming-mode-in-ReorderBuffer", otherwise, it is a bit
> difficult to review.
Actually, we can merge 0008, 0009, 0012, 0018 to the main patch
(0007).  Basically, if we merge all of them then we don't need to deal
with the conflict.  I think Tomas has kept them separate so that we
can review the solution for the schema sent.  And, I kept 0018 as a
separate patch to avoid conflict and rebasing in 0008, 0009 and 0012.
In the next patch set, I will merge all of them to 0007.

>
> > > > 0002-Issue-individual-invalidations-with-wal_level-log
> > > > 
> > > > 1.
> > > > xact_desc_invalidations(StringInfo buf,
> > > > {
> > > > ..
> > > > + else if (msg->id == SHAREDINVALSNAPSHOT_ID)
> > > > + appendStringInfo(buf, " snapshot %u", msg->sn.relId);
> > > >
> > > > You have removed logging for the above cache but forgot to remove its
> > > > reference from one of the places.  Also, I think you need to add a
> > > > comment somewhere in inval.c to say why you are writing for WAL for
> > > > some types of invalidations and not for others?
> > Done
> >
>
> I don't see any new comments as asked by me.
Oh, I just fixed one part of the comment and overlooked the rest.  Will fix.
  I think we should also
> consider WAL logging at each command end instead of doing piecemeal as
> discussed in another email [1], which will have lesser code changes
> and maybe better in performance.  You might want to evaluate the
> performance of both approaches.
Ok
>
> > > >
> > > > 0003-Extend-the-output-plugin-API-with-stream-methods
> > > > 
> > > >
> > > > 4.
> > > > stream_start_cb_wrapper()
> > > > {
> > > > ..
> > > > + /* state.report_location = apply_lsn; */
> > > > ..
> > > > + /* FIXME ctx->write_location = apply_lsn; */
> > > > ..
> > > > }
> > > >
> > > > See, if we can fix these and similar in the callback for the stop.  I
> > > > think we don't have final_lsn till we commit/abort.  Can we compute
> > > > before calling these API's?
> > Done
> >
>
> You have just used final_lsn, but I don't see where you have ensured
> that it is set before the API stream_stop_cb_wrapper.  I think we need
> something similar to what Vignesh has done in one of his bug-fix patch
> [2].  See my comment below in this regard.

You can refer below hunk in 0018.

+ /*
+ * Done with current changes, call stream_stop callback for streaming
+ * transaction, commit callback otherwise.
+ */
+ if (streaming)
+ {
+ /*
+ * Set the last last of the stream as the final lsn before calling
+ * stream stop.
+ */
+ txn->final_lsn = prev_lsn;
+ rb->stream_stop(rb, txn);
+ }

>
> > > >
> > > >
> > > > 0005-Gracefully-handle-concurrent-aborts-of-uncommitte
> > > > --
> > > > 1.
> > > > @@ -1877,6 +1877,7 @@ ReorderBufferCommit(ReorderBuffer *rb, 
> > > > TransactionId xid,
> > > >   PG_CATCH();
> > > >   {
> > > >   /* TODO: Encapsulate cleanup
> > > > from the PG_TRY and PG_CATCH blocks */
> > > > +
> > > >   if (iterstate)
> > > >   ReorderBufferIterTXNFinish(rb, iterstate);
> > > >
> > > > Spurious line change.
> > > >
> > Done
>
> + /*
> + * We don't expect direct calls to heap_getnext with valid
> + * CheckXidAlive for regular tables. Track that below.
> + */
> + if (unlikely(TransactionIdIsValid(CheckXidAlive) &&
> + !(IsCatalogRelation(scan->rs_base.rs_rd) ||
> +   RelationIsUsedAsCatalogTable(scan->rs_base.rs_rd
> + elog(ERROR, "improper heap_getnext call");
>
> Earlier, I thought we don't need to check if it is a regular table in
> this check, but it is required because output plugins can try to do
> that
I did not understand that, can you give some example?
and if they do so during decoding (with historic snapshots), the
> same should be not allowed.
>
> How about changing the error message to "

Re: doc: alter table references bogus table-specific planner parameters

[Simon Riggs]

On Mon, 6 Jan 2020 at 02:56, Justin Pryzby  wrote:

> commit 6f3a13ff058f15d565a30c16c0c2cb14cc994e42 Enhance docs for ALTER
> TABLE lock levels of storage parms
> Author: Simon Riggs 
> Date:   Mon Mar 6 16:48:12 2017 +0530
>
> 
>  SET (  class="PARAMETER">storage_parameter =  class="PARAMETER">value [, ... ] )
> ...
> -  Changing fillfactor and autovacuum storage parameters acquires a
> SHARE UPDATE EXCLUSIVE lock.
> +  SHARE UPDATE EXCLUSIVE lock will be taken for
> +  fillfactor and autovacuum storage parameters, as well as the
> +  following planner related parameters:
> +  effective_io_concurrency, parallel_workers, seq_page_cost
> +  random_page_cost, n_distinct and n_distinct_inherited.
>
> effective_io_concurrency, seq_page_cost and random_page_cost cannot be set
> for
> a table - reloptions.c shows that they've always been
> RELOPT_KIND_TABLESPACE.
>

Right, but if they were settable at table-level, the lock levels shown
would be accurate.

I agree with the sentiment of the third doc change, but your patch removes
the mention of n_distinct, which isn't appropriate.

The second change in your patch alters the meaning of the sentence in a way
that is counter to the first change. The name of these parameters is
"Storage Parameters" (in various places); I might agree with describing
them in text as "storage or planner parameters", but if you do that you
can't then just refer to "storage parameters" later, because if you do it
implies that planner parameters operate differently to storage parameters,
which they don't.


> n_distinct lock mode seems to have been changed and documented at e5550d5f
> ;
> 21d4e2e2 claimed to do the same, but the LOCKMODE is never used.
>

But neither does it need to because we don't lock tablespaces.

Thanks for your comments.

-- 
Simon Riggshttp://www.2ndQuadrant.com/

PostgreSQL Solutions for the Enterprise

doc: alter table references bogus table-specific planner parameters

[Justin Pryzby]

commit 6f3a13ff058f15d565a30c16c0c2cb14cc994e42 Enhance docs for ALTER TABLE 
lock levels of storage parms
Author: Simon Riggs 
Date:   Mon Mar 6 16:48:12 2017 +0530


 SET ( storage_parameter = value [, ... ] )
...
-  Changing fillfactor and autovacuum storage parameters acquires a 
SHARE UPDATE EXCLUSIVE lock.
+  SHARE UPDATE EXCLUSIVE lock will be taken for 
+  fillfactor and autovacuum storage parameters, as well as the
+  following planner related parameters:
+  effective_io_concurrency, parallel_workers, seq_page_cost
+  random_page_cost, n_distinct and n_distinct_inherited.

effective_io_concurrency, seq_page_cost and random_page_cost cannot be set for
a table - reloptions.c shows that they've always been RELOPT_KIND_TABLESPACE.

n_distinct lock mode seems to have been changed and documented at e5550d5f ;
21d4e2e2 claimed to do the same, but the LOCKMODE is never used.

See also:

commit 21d4e2e20656381b4652eb675af4f6d65053607f Reduce lock levels for table 
storage params related to planning
Author: Simon Riggs 
Date:   Mon Mar 6 16:04:31 2017 +0530

commit 47167b7907a802ed39b179c8780b76359468f076 Reduce lock levels for ALTER 
TABLE SET autovacuum storage options
Author: Simon Riggs 
Date:   Fri Aug 14 14:19:28 2015 +0100

commit e5550d5fec66aa74caad1f79b79826ec64898688 Reduce lock levels of some 
ALTER TABLE cmds
Author: Simon Riggs 
Date:   Sun Apr 6 11:13:43 2014 -0400

commit 2dbbda02e7e688311e161a912a0ce00cde9bb6fc Reduce lock levels of CREATE 
TRIGGER and some ALTER TABLE, CREATE RULE actions.
Author: Simon Riggs 
Date:   Wed Jul 28 05:22:24 2010 +

commit d86d51a95810caebcea587498068ff32fe28293e Support ALTER TABLESPACE name 
SET/RESET ( tablespace_options ).
Author: Robert Haas 
Date:   Tue Jan 5 21:54:00 2010 +

Justin
>From 64699ee90ef6ebe9459e3b2b1f603f30ec2c49c8 Mon Sep 17 00:00:00 2001
From: Justin Pryzby 
Date: Sun, 5 Jan 2020 19:39:29 -0600
Subject: [PATCH v1] Fixes for commit 6f3a13ff

Should backpatch to v10.
---
 doc/src/sgml/ref/alter_table.sgml | 8 +++-
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index bb1e48a..e5f39c2 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -676,32 +676,30 @@ WITH ( MODULUS numeric_literal, REM

 

 SET ( storage_parameter = value [, ... ] )
 
  
-  This form changes one or more storage parameters for the table.  See
+  This form changes one or more storage or planner parameters for the table.  See
   
   for details on the available parameters.  Note that the table contents
-  will not be modified immediately by this command; depending on the
+  will not be modified immediately by setting its storage parameters; depending on the
   parameter you might need to rewrite the table to get the desired effects.
   That can be done with VACUUM
   FULL,  or one of the forms
   of ALTER TABLE that forces a table rewrite.
   For planner related parameters, changes will take effect from the next
   time the table is locked so currently executing queries will not be
   affected.
  
 
  
   SHARE UPDATE EXCLUSIVE lock will be taken for
   fillfactor, toast and autovacuum storage parameters, as well as the
-  following planner related parameters:
-  effective_io_concurrency, parallel_workers, seq_page_cost,
-  random_page_cost, n_distinct and n_distinct_inherited.
+  parallel_workers planner parameter.
  
 

 

 RESET ( storage_parameter [, ... ] )
-- 
2.7.4

Re: Removal of support for OpenSSL 0.9.8 and 1.0.0

[Michael Paquier]

On Thu, Jan 02, 2020 at 09:22:47AM -0500, Tom Lane wrote:
> FWIW, I'm not sure I see why there's a connection between moving up
> the minimum Python version and minimum OpenSSL version.  Nobody is
> installing bleeding-edge Postgres on RHEL5, not even me ;-), so I
> don't especially buy Peter's line of reasoning.

It seems to me that the line of reasoning was to consider RHEL5 in the
garbage for all our dependencies, in a consistent way.

> I'm perfectly okay with doing both things in HEAD, I just don't
> see that doing one is an argument for or against doing the other.

Yes, right.  That would be the case if we had direct dependencies
between both, but that has never been the case AFAIK.
--
Michael


signature.asc
Description: PGP signature

Re: Commit fest manager for 2020-01

[Michael Paquier]

On Sun, Jan 05, 2020 at 04:55:18AM +0100, Tomas Vondra wrote:
> On Sat, Jan 04, 2020 at 06:19:33PM -0500, Tom Lane wrote:
>> I'd say you're it, since you volunteered first;
> 
> OK

Sounds like a plan then.  

> I'm not against doing that, but I don't know how to split the work.

You could also be two independent entities when it comes to review
patches.  There are so many entries that when it comes to
classification of the patches there is unlikely going to be a
conflict.  I'll try to do my own share of patches to look at, but
that's business as usual.
--
Michael


signature.asc
Description: PGP signature

Re: Assert failure due to "drop schema pg_temp_3 cascade" for temporary tables and \d+ is not showing any info after drooping temp table schema

[Michael Paquier]

On Sun, Dec 29, 2019 at 07:37:15AM -0500, Robert Haas wrote:
> I think this was a bad idea and that it should be reverted. It seems
> to me that the problem here is that you introduced a feature which had
> a bug, namely that it couldn't tolerate concurrency, and when somebody
> discovered the bug, you "fixed" it not by making the code able to
> tolerate concurrent activity but by preventing concurrent activity
> from happening in the first place. I think that's wrong on general
> principle.

Sorry for the delay, there was a long period off here so I could not
have a serious look.

The behavior of the code in 246a6c8 has changed so as a non-existing
temporary namespace is considered as not in use, in which case
autovacuum would consider this relation to be orphaned, and it would
then try to drop it.  Anyway, just a revert of the patch is not a good
idea either, because keeping around the old behavior allows any user
to create orphaned relations that autovacuum would just ignore in
9.4~10, leading to ultimately a forced shutdown of the instance as no
cleanup can happen if this goes unnoticed.  This also puts pg_class
into an inconsistent state as pg_class entries would include
references to a namespace that does not exist for sessions still
holding its own references to myTempNamespace/myTempToastNamespace.

> In this specific case, DROP SCHEMA on another temporary sessions's
> schema is a feature which has existed for a very long time and which I
> have used on multiple occasions to repair damaged databases. Suppose,
> for example, there's a catalog entry that prevents the schema from
> being dropped. Before this commit, you could fix it or delete the
> entry and then retry the drop. Now, you can't. You can maybe wait for
> autovacuum to retry it or something, assuming autovacuum is working
> and you're in multi-user mode.

This behavior is broken since its introduction then per the above.  If
we were to allow DROP SCHEMA to work properly on temporary schema, we
would need to do more than what we have now, and that does not involve
just mimicking DISCARD TEMP if you really wish to be able to drop the
schema entirely and not only the objects it includes.  Allowing a
temporary schema to be dropped only if it is owned by the current
session would be simple enough to implement, but I think that allowing
that to work properly for a schema owned by another session would be
rather difficult to implement for little gains.  Now, if you still
wish to be able to do a DROP SCHEMA on a temporary schema, I have no
objections to allow doing that, but under some conditions.  So I would
recommend to restrict it so as this operation is not allowed by
default, and I think we ought to use allow_system_table_mods to
control that, because if you were to do that you are an operator and
you know what you are doing.  Normally :)

> But even if that weren't the case, this seems like a very fragile fix.
> Maybe someday we'll allow multiple autovacuum workers in the same
> database, and the problem comes back. Maybe some user who can't drop
> the schema because of this arbitrary prohibition will find themselves
> forced to delete the pg_namespace row by hand and then crash the
> server. Most server code is pretty careful that to either tolerate
> missing system catalog tuples or elog(ERROR), not crash (e.g. cache
> lookup failed for ...). This code shouldn't be an exception to that
> rule.

You are right here, things could be done better in 11 and newer
versions, still there are multiple ways to do that.  Here are three
suggestions:
1) Issue an elog(ERROR) as that's what we do usually for lookup errors
and such when seeing an orphaned relation which refers to a
non-existing namespace.  But this would prevent autovacuum to do
any kind of work and just loop over-and-over on the same error, just
bloating the database involved.
2) Ignore the relation and leave it around, though we really have been
fighting to make autovacuum more aggressive, so that would defeat the
work done lately for that purpose.
3) Still drop the orphaned relation even if it references to a
non-existing schema, generating an appropriate LOG message so as the
problem comes from an incorrect lookup at the namespace name.

Attached is a patch doing two things:
a) Control DROP SCHEMA on a temporary namespace using
allow_system_table_mods.
b) Generate a non-buggy LOG message if trying to remove a temp
relation referring to a temporary schema that does not exist, using
"(null)" as a replacement for the schema name.

My suggestion is to do a) down to 9.4 if that's thought to be helpful
to have, and at least Robert visibly thinks so, then b) in 11~ as
that's where 246a6c8 exists.  Comments welcome.
--
Michael
diff --git a/src/backend/commands/dropcmds.c b/src/backend/commands/dropcmds.c
index e7891a4418..725e395bbe 100644
--- a/src/backend/commands/dropcmds.c
+++ b/src/backend/commands/dropcmds.c
@@ -109,7 +109,8 @@ RemoveObjects(DropStmt *stmt)
 		 * result in inconsistenc

RE: Libpq support to connect to standby server as priority

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

From: Alvaro Herrera 
> So, we can know whether server is primary/standby by checking
> in_recovery, as opposed to knowing whether read-write which is done by
> checking transaction_read_only.  So we can keep read-write as a synonym
> for "primary", and check in_recovery when used in servers that support
> the new GUC, and check transaction_read_only in older servers.
> 
> It seems there's a lot of code that we can discard from the patch:
> first, we can discard checking for "read-only" altogether.  Second, have
> us check transaction_read_only *only* if the server is of an older
> version.

Let me check my understanding.  Are you proposing these?

* The canonical libpq connection parameter is target_session_attr = {primary | 
standby | prefer-standby}.  Leave and document read-write as a synonym for 
primary.

* When the server version is 13 or later, libpq just checks in_recovery, not 
checking transaction_read_only or sending SHOW transaction_read_only.

* When the server version is before 13, libpq sends SHOW transaction_read_only 
as before.


Personally, 100% agreed, considering what we really wanted to do when 
target_session_attr was introduced is to tell if the server is primary or 
standby.  The questions are:

Q1: Should we continue to use the name target_session_attr, or rename it to 
target_server_type and make target_session_attr a synonym for it?  I'm in favor 
of the latter.

Q2: Can we accept the subtle incompatibility that 
target_session_attr=read-write and target_server_type=primary are not the same, 
when default_transaction_read_only is on?  (I'd like to hear yes)

Q3: Can we go without supporting standby and prefer-standby for older servers?  
(I think yes because we can say that it's a new feature effective for new 
servers.)


Regards
Takayuki Tsunakawa

Re: Removing pg_pltemplate and creating "trustable" extensions

[Tom Lane]

I wrote:
> The cfbot noticed that a couple of patches committed this week
> created (trivial) conflicts with this patchset.  Here's a v3
> rebased up to HEAD; no interesting changes.

The 2020 copyright update broke this patchset again.  Here's a rebase.
No changes except for some minor rearrangement of the CREATE LANGUAGE
man page in 0003.

regards, tom lane

diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 55694c4..a573dfb 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -8514,7 +8514,15 @@ SCRAM-SHA-256$:&l
  
   superuser
   bool
-  True if only superusers are allowed to install this extension
+  True if only superusers are allowed to install this extension
+   (but see trusted)
+ 
+
+ 
+  trusted
+  bool
+  True if the extension can be installed by non-superusers
+   with appropriate privileges
  
 
  
diff --git a/doc/src/sgml/extend.sgml b/doc/src/sgml/extend.sgml
index a3046f2..e2807d0 100644
--- a/doc/src/sgml/extend.sgml
+++ b/doc/src/sgml/extend.sgml
@@ -576,6 +576,32 @@
 version.  If it is set to false, just the privileges
 required to execute the commands in the installation or update script
 are required.
+This should normally be set to true if any of the
+script commands require superuser privileges.  (Such commands would
+fail anyway, but it's more user-friendly to give the error up front.)
+   
+  
+ 
+
+ 
+  trusted (boolean)
+  
+   
+This parameter, if set to true (which is not the
+default), allows some non-superusers to install an extension that
+has superuser set to true.
+Specifically, installation will be permitted for the owner of the
+current database, and for anyone who has been granted
+the pg_install_trusted_extension role.
+When the user executing CREATE EXTENSION is not
+a superuser but is allowed to install by virtue of this parameter,
+then the installation or update script is run as the bootstrap
+superuser, not as the calling user.
+This parameter is irrelevant if superuser is
+false.
+Generally, this should not be set true for extensions that could
+allow access to otherwise-superuser-only abilities, such as
+filesystem access.

   
  
@@ -642,6 +668,18 @@
 
 
 
+ If the extension script contains the
+ string @extowner@, that string is replaced with the
+ (suitably quoted) name of the user calling CREATE
+ EXTENSION or ALTER EXTENSION.  Typically
+ this feature is used by extensions that are marked trusted to assign
+ ownership of selected objects to the calling user rather than the
+ bootstrap superuser.  (One should be careful about doing so, however.
+ For example, assigning ownership of a C-language function to a
+ non-superuser would create a privilege escalation path for that user.)
+
+
+
  While the script files can contain any characters allowed by the specified
  encoding, control files should contain only plain ASCII, because there
  is no way for PostgreSQL to know what encoding a
diff --git a/doc/src/sgml/ref/create_extension.sgml b/doc/src/sgml/ref/create_extension.sgml
index 36837f9..7cd4346 100644
--- a/doc/src/sgml/ref/create_extension.sgml
+++ b/doc/src/sgml/ref/create_extension.sgml
@@ -47,14 +47,26 @@ CREATE EXTENSION [ IF NOT EXISTS ] extension_name
   
 
   
-   Loading an extension requires the same privileges that would be
-   required to create its component objects.  For most extensions this
-   means superuser or database owner privileges are needed.
The user who runs CREATE EXTENSION becomes the
owner of the extension for purposes of later privilege checks, as well
as the owner of any objects created by the extension's script.
   
 
+  
+   Loading an extension ordinarily requires the same privileges that would
+   be required to create its component objects.  For many extensions this
+   means superuser privileges are needed.
+   However, if the extension is marked trusted in
+   its control file, then it can be installed by a non-superuser who has
+   suitable privileges (that is, owns the current database or has been
+   granted the pg_install_trusted_extension role).  In
+   this case the extension object itself will be owned by the calling user,
+   but the contained objects will be owned by the bootstrap superuser
+   (unless the extension's script explicitly assigns them to the calling
+   user).  This configuration gives the calling user the right to drop the
+   extension, but not to modify individual objects within it.
+  
+
  
 
  
diff --git a/doc/src/sgml/user-manag.sgml b/doc/src/sgml/user-manag.sgml
index 66f1627..90f637f 100644
--- a/doc/src/sgml/user-manag.sgml
+++ b/doc/src/sgml/user-manag.sgml
@

Re: [Proposal] Global temporary tables

[Tomas Vondra]

Hi,

I think we need to do something with having two patches aiming to add
global temporary tables:

[1] https://commitfest.postgresql.org/26/2349/

[2] https://commitfest.postgresql.org/26/2233/

As a reviewer I have no idea which of the threads to look at - certainly
not without reading both threads, which I doubt anyone will really do.
The reviews and discussions are somewhat intermixed between those two
threads, which makes it even more confusing.

I think we should agree on a minimal patch combining the necessary/good
bits from the various patches, and terminate one of the threads (i.e.
mark it as rejected or RWF). And we need to do that now, otherwise
there's about 0% chance of getting this into v13.

In general, I agree with the sentiment Rober expressed in [1] - the
patch needs to be as small as possible, not adding "nice to have"
features (like support for parallel queries - I very much doubt just
using shared instead of local buffers is enough to make it work.)

regards

--
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: WIP: System Versioned Temporal Table

[legrand legrand]

Vik Fearing-4 wrote
> On 05/01/2020 16:01, legrand legrand wrote:
> 
> 
> No, that is incorrect.  The standard syntax is:
> 
> 
>     FROM tablename FOR SYSTEM_TIME AS OF '...'
> 
>     FROM tablename FOR SYSTEM_TIME BETWEEN '...' AND '...'
> 
>     FROM tablename FOR SYSTEM_TIME FROM '...' TO '...'
> 
> -- 
> 
> Vik Fearing

oups, I re-read links and docs and I'm wrong.
Sorry for the noise

Regards
PAscal



--
Sent from: https://www.postgresql-archive.org/PostgreSQL-hackers-f1928748.html

Re: Rethinking opclass member checks and dependency strength

[Tomas Vondra]

On Sun, Jan 05, 2020 at 12:33:10PM -0500, Tom Lane wrote:

Tomas Vondra  writes:

The latest version of this patch (from 2019/09/14) no longer applies,
although maybe it's some issue with patch format (applying it using
patch works fine, git am fails with "Patch format detection failed.").


Hm, seems to be just a trivial conflict against the copyright-date-update
patch.  Updated version attached.



Interesting. I still get

  $ git am ~/am-check-members-callback-3.patch
  Patch format detection failed.

I'm on git 2.21.1, not sure if that matters. Cputube is happy, though.

Meh.


On Sat, Sep 14, 2019 at 07:01:33PM -0400, Tom Lane wrote:

This change results in a possibly surprising change in the expected output
for the 002_pg_dump.pl test: an optional support function that had been
created as part of CREATE OPERATOR CLASS will now be dumped as part of
ALTER OPERATOR FAMILY.  Maybe that's too surprising?  Another approach
that we could use is to give up the premise that soft dependencies are
always on the opfamily.  If we kept the dependencies pointing to the
same objects as before (opclass or opfamily) and only twiddled the
dependency strength, then pg_dump's output would not change.  Now,
this would possibly result in dropping a still-useful family member
if it were incorrectly tied to an opclass that's dropped --- but that
would have happened before, too.  I'm not quite sure if we really want
to editorialize on the user's decisions about which grouping to tie
family members to.



I agree it's a bit weird to add a dependency on an opfamily and not the
opclass. Not just because of the pg_dump weirdness, but doesn't it mean
that after a DROP OPERATOR CLASS we might still reject a DROP OPERATOR
because of the remaining opfamily dependency? (Haven't tried, so maybe
that works fine).


I poked at the idea of retaining the user's decisions as to whether
a member object is a member of an individual opclass or an opfamily,
but soon realized that there's a big problem with that: we don't have
any ALTER OPERATOR CLASS ADD/DROP syntax, only ALTER OPERATOR FAMILY.
So there's no way to express the concept of "add this at the opclass
level", if you forgot to add it during initial opclass creation.

I suppose that some case could be made for adding such syntax, but
it seems like a significant amount of work, and in the end it seems
like it's better to trust the system to get these assignments right.
Letting the user do it doesn't add much except the opportunity
to shoot oneself in the foot.



OK. So we shall keep the v2 behavior, with opfamily dependencies and
modified pg_dump output? Fine with me - I still think it's a bit weird,
but I'm willing to commit myself to add the missing syntax. And I doubt
anyone will notice, probably ...


One minor comment from me is that maybe "amcheckmembers" is a bit
misleading. In my mind "check" implies a plain passive check, not
something that may actually tweak the dependency type.


Hmm.  I'm not wedded to that name, but do you have a better proposal?
The end goal (not realized in this patch, of course) is that these
callbacks would perform fairly thorough checking of opclass members,
missing only the ability to check that all required members are present.
So I don't want to name them something like "amfixdependencies", even
if that's all they're doing right now.



OK.



I see your point that "check" suggests a read-only operation, but
I'm not sure about a better verb.  I thought of "amvalidatemembers",
but that's not really much better than "check" is it?



I don't :-(


regards

--
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Planning counters in pg_stat_statements (using pgss_store)

[Julien Rouhaud]

On Sun, Jan 5, 2020 at 7:02 PM legrand legrand
 wrote:
>
> Julien Rouhaud wrote
> > On Sun, Jan 5, 2020 at 4:11 PM legrand legrand
> > <
>
> > legrand_legrand@
>
> > > wrote:
> >>
> >> Hi Julien,
> >>
> >> I would like to create a link with
> >> https://www.postgresql.org/message-id/
>
> > 1577490124579-0.post@.nabble
>
> >>
> >> where we met an ASSET FAILURE because query text was not initialized ...
> >>
> >> The question raised is:
> >>
> >> - should query text be always provided
> >> or
> >> - if not how to deal that case (in pgss).
> >
> > I'd think that since the query text was until now always provided,
> > there's no reason why this patch should change that.  That being said,
> > there has been other concerns raised wrt. temporary tables in the IVM
> > patchset, so ISTM that there might be important architectural changes
> > upcoming, so having to deal with this case in pgss is not rushed
> > (especially since handling that in pgss would be trivial), and can
> > help to catch issue with the query text pasing.
>
> IVM revealed that ASSERT,
> but IVM works fine with pg_stat_statements.track_planning = off.

Yes, but on the other hand the current IVM patchset also adds the only
pg_plan_query call that don't provide a query text.  I didn't see any
other possibility, and if there are other cases they're unfortunately
not covered by the full regression tests.

> There may be others parts of postgresql that would have workede fine as
> well.
>
> This means 2 things:
> - there is a (litle) risk to meet other assert failures when using planning
> counters in pgss,
> - we have an easy workarround to fix it (disabling track_planning).
>
> But I would have prefered this new feature to work the same way with or
> without track_planning activated ;o(

Definitely, but fixing the issue in pgss (ignoring planner calls when
we don't have a query text) means that pgss won't give an exhaustive
view of activity anymore, so a fix in IVM would be a better solution.
Let's wait and see if Nagata-san and other people involved in that
have an opinion on it.

Re: pgsql: Add basic TAP tests for psql's tab-completion logic.

[Tom Lane]

I wrote:
> Indeed.  It appears that recent libedit breaks tab-completion for
> words involving a backslash, which is the fault of this upstream
> commit:
> http://cvsweb.netbsd.org/bsdweb.cgi/src/lib/libedit/filecomplete.c.diff?r1=1.52&r2=1.53
> Basically what that's doing is applying de-backslashing to EVERY
> word that completion is attempted on, whether it might be a filename
> or not.  So what psql_complete sees in this test case is just "DRD"
> which of course it does not recognize as a possible psql backslash
> command.

The current state of play on this is that I committed a hacky workaround
[1], but there is now a fix for it in libedit upstream [2][3].  I gather
from looking at Debian's package page that the fix could be expected to
propagate to Debian unstable within a few weeks, at which point I'd like
to revert the hack.  The libedit bug's only been there a few months
(it was evidently introduced on 2019-03-31) so we can hope that it hasn't
propagated into any long-term-support distros.

> I found out while investigating this that the libedit version shipping
> with buster (3.1-20181209) is differently broken for the same case:
> instead of inapproriate forced de-escaping of the input of the
> application-specific completion function, it applies inapproriate
> forced escaping to the output of said function, so that when we see
> "\DRD" and return "\drds", what comes out to the user is "\\drds".

There's little we can do about that one, but it doesn't affect the
regression test as currently constituted.

Another libedit bug that the regression test *is* running into is that
some ancient versions fail to emit a trailing space after a successful
completion.  snapper is hitting that [4], and I believe locust would
be if it were running the TAP tests.  While we could work around that
by removing the trailing spaces from the match regexps, I really
don't wish to do so, because that destroys the test's ability to
distinguish correct outputs from incorrect-but-longer ones.  (That's
not a killer problem for any of the current test cases, perhaps, but
I think it will be in future.)  So I'd like to define this problem as
being out of scope.  This bug was fixed eleven years ago upstream
(see change in _rl_completion_append_character_function in [5]), so
it seems reasonable to insist that people get a newer libedit or not
run this test.

Another issue visible in [4] is that ancient libedit fails to sort
the offered completion strings as one would expect.  I don't see
much point in working around that either.  prairiedog's host has
that bug but not the space bug (see [6], from before I suppressed
running the test on that machine), so it affects a larger range of
libedit versions, but they're probably all way too old for anyone
to care.  If anyone can point to a new-enough-to-matter libedit
that still behaves that way, we can reconsider.

regards, tom lane

[1] 
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=ddd87d564508bb1c80aac0a4439cfe74a3c203a9
[2] http://gnats.netbsd.org/cgi-bin/query-pr-single.pl?number=54510
[3] 
http://cvsweb.netbsd.org/bsdweb.cgi/src/lib/libedit/filecomplete.c.diff?r1=1.63&r2=1.64
[4] 
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=snapper&dt=2020-01-05%2013%3A01%3A46
[5] 
http://cvsweb.netbsd.org/bsdweb.cgi/src/lib/libedit/readline.c.diff?r1=1.75&r2=1.76
[6] 
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=prairiedog&dt=2020-01-02%2022%3A52%3A36

Re: Planning counters in pg_stat_statements (using pgss_store)

[legrand legrand]

Julien Rouhaud wrote
> On Sun, Jan 5, 2020 at 4:11 PM legrand legrand
> <

> legrand_legrand@

> > wrote:
>>
>> Hi Julien,
>>
>> I would like to create a link with
>> https://www.postgresql.org/message-id/

> 1577490124579-0.post@.nabble

>>
>> where we met an ASSET FAILURE because query text was not initialized ...
>>
>> The question raised is:
>>
>> - should query text be always provided
>> or
>> - if not how to deal that case (in pgss).
> 
> I'd think that since the query text was until now always provided,
> there's no reason why this patch should change that.  That being said,
> there has been other concerns raised wrt. temporary tables in the IVM
> patchset, so ISTM that there might be important architectural changes
> upcoming, so having to deal with this case in pgss is not rushed
> (especially since handling that in pgss would be trivial), and can
> help to catch issue with the query text pasing.

IVM revealed that ASSERT,
but IVM works fine with pg_stat_statements.track_planning = off.
There may be others parts of postgresql that would have workede fine as
well.

This means 2 things:
- there is a (litle) risk to meet other assert failures when using planning
counters in pgss,
- we have an easy workarround to fix it (disabling track_planning).

But I would have prefered this new feature to work the same way with or
without track_planning activated ;o(



--
Sent from: https://www.postgresql-archive.org/PostgreSQL-hackers-f1928748.html

Re: Rethinking opclass member checks and dependency strength

[Tom Lane]

Tomas Vondra  writes:
> The latest version of this patch (from 2019/09/14) no longer applies,
> although maybe it's some issue with patch format (applying it using
> patch works fine, git am fails with "Patch format detection failed.").

Hm, seems to be just a trivial conflict against the copyright-date-update
patch.  Updated version attached.

> On Sat, Sep 14, 2019 at 07:01:33PM -0400, Tom Lane wrote:
>> This change results in a possibly surprising change in the expected output
>> for the 002_pg_dump.pl test: an optional support function that had been
>> created as part of CREATE OPERATOR CLASS will now be dumped as part of
>> ALTER OPERATOR FAMILY.  Maybe that's too surprising?  Another approach
>> that we could use is to give up the premise that soft dependencies are
>> always on the opfamily.  If we kept the dependencies pointing to the
>> same objects as before (opclass or opfamily) and only twiddled the
>> dependency strength, then pg_dump's output would not change.  Now,
>> this would possibly result in dropping a still-useful family member
>> if it were incorrectly tied to an opclass that's dropped --- but that
>> would have happened before, too.  I'm not quite sure if we really want
>> to editorialize on the user's decisions about which grouping to tie
>> family members to.

> I agree it's a bit weird to add a dependency on an opfamily and not the
> opclass. Not just because of the pg_dump weirdness, but doesn't it mean
> that after a DROP OPERATOR CLASS we might still reject a DROP OPERATOR
> because of the remaining opfamily dependency? (Haven't tried, so maybe
> that works fine).

I poked at the idea of retaining the user's decisions as to whether
a member object is a member of an individual opclass or an opfamily,
but soon realized that there's a big problem with that: we don't have
any ALTER OPERATOR CLASS ADD/DROP syntax, only ALTER OPERATOR FAMILY.
So there's no way to express the concept of "add this at the opclass
level", if you forgot to add it during initial opclass creation.

I suppose that some case could be made for adding such syntax, but
it seems like a significant amount of work, and in the end it seems
like it's better to trust the system to get these assignments right.
Letting the user do it doesn't add much except the opportunity
to shoot oneself in the foot.

> One minor comment from me is that maybe "amcheckmembers" is a bit
> misleading. In my mind "check" implies a plain passive check, not
> something that may actually tweak the dependency type.

Hmm.  I'm not wedded to that name, but do you have a better proposal?
The end goal (not realized in this patch, of course) is that these
callbacks would perform fairly thorough checking of opclass members,
missing only the ability to check that all required members are present.
So I don't want to name them something like "amfixdependencies", even
if that's all they're doing right now.

I see your point that "check" suggests a read-only operation, but
I'm not sure about a better verb.  I thought of "amvalidatemembers",
but that's not really much better than "check" is it?

regards, tom lane

diff --git a/contrib/bloom/blutils.c b/contrib/bloom/blutils.c
index 23d959b..b016be1 100644
--- a/contrib/bloom/blutils.c
+++ b/contrib/bloom/blutils.c
@@ -134,6 +134,7 @@ blhandler(PG_FUNCTION_ARGS)
 	amroutine->amproperty = NULL;
 	amroutine->ambuildphasename = NULL;
 	amroutine->amvalidate = blvalidate;
+	amroutine->amcheckmembers = NULL;
 	amroutine->ambeginscan = blbeginscan;
 	amroutine->amrescan = blrescan;
 	amroutine->amgettuple = NULL;
diff --git a/doc/src/sgml/indexam.sgml b/doc/src/sgml/indexam.sgml
index dd54c68..2d16c6f 100644
--- a/doc/src/sgml/indexam.sgml
+++ b/doc/src/sgml/indexam.sgml
@@ -137,6 +137,7 @@ typedef struct IndexAmRoutine
 amproperty_function amproperty; /* can be NULL */
 ambuildphasename_function ambuildphasename;   /* can be NULL */
 amvalidate_function amvalidate;
+amcheckmembers_function amcheckmembers; /* can be NULL */
 ambeginscan_function ambeginscan;
 amrescan_function amrescan;
 amgettuple_function amgettuple; /* can be NULL */
@@ -496,7 +497,48 @@ amvalidate (Oid opclassoid);
the access method can reasonably do that.  For example, this might include
testing that all required support functions are provided.
The amvalidate function must return false if the opclass is
-   invalid.  Problems should be reported with ereport messages.
+   invalid.  Problems should be reported with ereport
+   messages, typically at INFO level.
+  
+
+  
+
+void
+amcheckmembers (Oid opfamilyoid,
+Oid opclassoid,
+List *operators,
+List *functions);
+
+   Validate proposed operator and function members of an operator family,
+   so far as the access method can reasonably do that, and set their
+   dependency types if the default is not satisfactory.  This is called
+   during CREATE OPERATOR CLASS and during

Re: pgsql: Add basic TAP tests for psql's tab-completion logic.

[Dagfinn Ilmari Mannsåker]

On 5 January 2020 16:38:36 GMT, Tom Lane  wrote:
>ilm...@ilmari.org (Dagfinn Ilmari =?utf-8?Q?Manns=C3=A5ker?=) writes:
>> Here's one final style cleanup for the TAP test.
>
>LGTM, pushed.

Thanks!

>One minor note: you wanted to change the \DRD test to
>
>+check_completion("\\DRD\t", qr/\\drds /, "complete \\DRD to
>\\drds");
>
>but that doesn't work everywhere, unfortunately.  On my machine
>what comes out is
>
># Actual output was "\\DRD\b\b\bdrds "
># Did not match "(?-xism:\\drds )"

Sorry, that was something I left in for testing the diagnostic and forgot to 
remove before committing.

>   regards, tom lane

- ilmari

Re: pgsql: Add basic TAP tests for psql's tab-completion logic.

[Tom Lane]

ilm...@ilmari.org (Dagfinn Ilmari =?utf-8?Q?Manns=C3=A5ker?=) writes:
> Here's one final style cleanup for the TAP test.

LGTM, pushed.

One minor note: you wanted to change the \DRD test to

+check_completion("\\DRD\t", qr/\\drds /, "complete \\DRD to \\drds");

but that doesn't work everywhere, unfortunately.  On my machine
what comes out is

# Actual output was "\\DRD\b\b\bdrds "
# Did not match "(?-xism:\\drds )"

regards, tom lane

Re: FETCH FIRST clause WITH TIES option

[Tomas Vondra]

On Fri, Nov 29, 2019 at 05:19:00AM +, Andrew Gierth wrote:

"Alvaro" == Alvaro Herrera  writes:


Alvaro> First, I noticed that there's a significant unanswered issue
Alvaro> from Andrew Gierth about this using a completely different
Alvaro> mechanism, namely an implicit window function. Robert was
Alvaro> concerned about the performance of Andrew's proposed approach,
Alvaro> but I don't see any reply from Surafel on the whole issue.

Alvaro> Andrew: Would you rather not have this feature in this form at
Alvaro> all?

I have done a proof-of-concept hack for my alternative approach, which I
will post in another thread so as not to confuse the cfbot.

The main advantage, as I see it, of a window function approach is that
it can also work for PERCENT with only a change to the generated
expression; the executor part of the code can handle any stopping
condition. It can also be generalized (though the POC does not do this)
to allow an SQL-level extension, something like "LIMIT WHEN condition"
to indicate that it should stop just before the first row for which the
condition is true. Whether this is a desirable feature or not is another
question.



For the record, the alternative approach was posted here:

https://www.postgresql.org/message-id/flat/87o8wvz253@news-spur.riddles.org.uk

I think we need to make a decision about which road to take - shall we
continue with what Surafel originally proposed, or should we instead
adopt Andrew's patch?

Andrew's patch looks significantly simpler, although it probably will
need more work to get it committable. My main concern about using window
functions was performance, so I think we should benchmark. For example,
considering window functions are not parallel safe, would this have
negative impact on parallel queries?


regards

--
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Planning counters in pg_stat_statements (using pgss_store)

[Julien Rouhaud]

On Sun, Jan 5, 2020 at 4:11 PM legrand legrand
 wrote:
>
> Hi Julien,
>
> I would like to create a link with
> https://www.postgresql.org/message-id/1577490124579-0.p...@n3.nabble.com
>
> where we met an ASSET FAILURE because query text was not initialized ...
>
> The question raised is:
>
> - should query text be always provided
> or
> - if not how to deal that case (in pgss).

I'd think that since the query text was until now always provided,
there's no reason why this patch should change that.  That being said,
there has been other concerns raised wrt. temporary tables in the IVM
patchset, so ISTM that there might be important architectural changes
upcoming, so having to deal with this case in pgss is not rushed
(especially since handling that in pgss would be trivial), and can
help to catch issue with the query text pasing.

Re: WIP: System Versioned Temporal Table

[Vik Fearing]

On 05/01/2020 16:01, legrand legrand wrote:
>
>> As for the syntax, you have:
>>
>>
>> select a from for stest0 system_time from '2000-01-01 00:00:00.0' to
>> 'infinity' ORDER BY a;
>>
>>
>> when you should have:
>>
>>
>> select a from stest0 for system_time from '2000-01-01 00:00:00.0' to
>> 'infinity' ORDER BY a;
>>
>>
>> That is, the FOR should be on the other side of the table name.
>>
>> [...] 
>>
>> Vik Fearing
> Hello,
>
> I though that standard syntax was "AS OF SYSTEM TIME"
> as discussed here
> https://www.postgresql.org/message-id/flat/A254CDC3-D308-4822-8928-8CC584E0CC71%40elusive.cx#06c5dbffd5cfb9a20cdeec7a54dc657f
> , also explaining how to parse such a syntax .


No, that is incorrect.  The standard syntax is:


    FROM tablename FOR SYSTEM_TIME AS OF '...'

    FROM tablename FOR SYSTEM_TIME BETWEEN '...' AND '...'

    FROM tablename FOR SYSTEM_TIME FROM '...' TO '...'

-- 

Vik Fearing

Re: Planning counters in pg_stat_statements (using pgss_store)

[legrand legrand]

Hi Julien,

I would like to create a link with
https://www.postgresql.org/message-id/1577490124579-0.p...@n3.nabble.com

where we met an ASSET FAILURE because query text was not initialized ...

The question raised is:

- should query text be always provided 
or 
- if not how to deal that case (in pgss).

Regards
PAscal



--
Sent from: https://www.postgresql-archive.org/PostgreSQL-hackers-f1928748.html

Re: WIP: System Versioned Temporal Table

[legrand legrand]

Vik Fearing-4 wrote
> On 05/01/2020 11:16, Surafel Temesgen wrote:
>>
>>
>> On Fri, Jan 3, 2020 at 4:22 PM Vik Fearing
>> <

> vik.fearing@

>   vik.fearing@

> >> wrote:
>>
> 
> [...]
> 
> You only test FROM-TO and with a really wide interval.  There are no
> tests for AS OF and no tests for BETWEEN-AND.
> 
> 
> As for the syntax, you have:
> 
> 
> select a from for stest0 system_time from '2000-01-01 00:00:00.0' to
> 'infinity' ORDER BY a;
> 
> 
> when you should have:
> 
> 
> select a from stest0 for system_time from '2000-01-01 00:00:00.0' to
> 'infinity' ORDER BY a;
> 
> 
> That is, the FOR should be on the other side of the table name.
> 
> [...] 
> 
> Vik Fearing

Hello,

I though that standard syntax was "AS OF SYSTEM TIME"
as discussed here
https://www.postgresql.org/message-id/flat/A254CDC3-D308-4822-8928-8CC584E0CC71%40elusive.cx#06c5dbffd5cfb9a20cdeec7a54dc657f
, also explaining how to parse such a syntax .

Regards
PAscal



--
Sent from: https://www.postgresql-archive.org/PostgreSQL-hackers-f1928748.html

Re: parallel vacuum options/syntax

[Amit Kapila]

On Sun, Jan 5, 2020 at 7:38 PM Masahiko Sawada
 wrote:
>
> On Sun, 5 Jan 2020 at 22:39, Tomas Vondra  
> wrote:
> >
> >
> > So if we think we need an option to determine vacuum parallel degree, we
> > should have an option to disable parallelism too. I don't care much if
> > it's called DISABLE_PARALLEL, NOPARALLEL or PARALLEL 0, as long as we
> > make our mind and don't unnecessarily break it in the next release.
> >

Fair point.  I favor parallel 0 as that avoids adding more options and
also it is not very clear whether that is required at all.  Till now,
if I see most people who have shared their opinion seems to favor this
as compared to another idea where we need to introduce more options.

>
> Okay I got your point. It's just an idea but how about controlling
> parallel vacuum using two options. That is, we have PARALLEL option
> that takes a boolean value (true by default) and enables/disables
> parallel vacuum. And we have WORKERS option that takes an integer more
> than 1 to specify the number of workers. Of course we should raise an
> error if only WORKERS option is specified. WORKERS option is optional.
> If WORKERS option is omitted the number of workers is determined based
> on the number of indexes on the table.
>

I think this would add failure modes without serving any additional
purpose.  Sure, it might give the better feeling that we have separate
options to enable/disable parallelism and then specify the number of
workers with a separate option, but we already have various examples
as shared by me previously where setting the value as zero means the
option is disabled, so why to invent something new here?


-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Re: parallel vacuum options/syntax

[Masahiko Sawada]

On Sun, 5 Jan 2020 at 22:39, Tomas Vondra  wrote:
>
> On Sun, Jan 05, 2020 at 09:17:57PM +0900, Masahiko Sawada wrote:
> >On Sun, Jan 5, 2020 at 7:26 PM Amit Kapila  wrote:
> >>
> >> ...
> >>
> >> > If we want to have a vacuum option to determine parallel degree, we
> >> > should probably have a vacuum option to disable parallelism using just a
> >> > vacuum option. I don't think 0 is too bad, and disable_parallel seems a
> >> > bit awkward. Maybe we could use NOPARALLEL (in addition to PARALLEL n).
> >> > That's what Oracle does, so it's not entirely without a precedent.
> >> >
> >>
> >> We can go either way (using 0 for parallel to indicate disable
> >> parallelism or by introducing a new option like NOPARALLEL).  I think
> >> initially we can avoid introducing more options and just go with
> >> 'Parallel 0' and if we find a lot of people find it inconvenient, then
> >> we can always introduce a new option later.
> >
> >Hmm I'm confused. Specifying NOPARALLEL or PARALLEL 0 is the same as
> >setting max_parallel_maintenance_workers to 0, right? We normally set
> >max_parallel_workers_per_gather to 0 to disable parallel queries on a
> >query. So I think that disabling parallel vacuum by setting
> >max_parallel_maintenance_workers to 0 is the same concept. Regarding
> >proposed two options we already have storage parameter
> >parallel_workers and it accepts 0 but PARALLEL 0 looks like
> >contradicted at a glance. And NOPARALLEL is inconsistent with existing
> >DISABLE_XXX options and it's a bit awkward to specify like (NOPARALLEL
> >off).
> >
>
> My understanding is the motivation for new vacuum options is a claim
> that m_p_m_w is not sufficient/suitable for the vacuum case. I've
> expressed my doubts about this, but let's assume it's the right
> solution. To me it seems a bit confusing to just fall back to m_p_m_w
> when it comes to disabling the parallel vacuum.
>
> So if we think we need an option to determine vacuum parallel degree, we
> should have an option to disable parallelism too. I don't care much if
> it's called DISABLE_PARALLEL, NOPARALLEL or PARALLEL 0, as long as we
> make our mind and don't unnecessarily break it in the next release.

Okay I got your point. It's just an idea but how about controlling
parallel vacuum using two options. That is, we have PARALLEL option
that takes a boolean value (true by default) and enables/disables
parallel vacuum. And we have WORKERS option that takes an integer more
than 1 to specify the number of workers. Of course we should raise an
error if only WORKERS option is specified. WORKERS option is optional.
If WORKERS option is omitted the number of workers is determined based
on the number of indexes on the table.

Regards,

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

Re: pgsql: Add basic TAP tests for psql's tab-completion logic.

[Dagfinn Ilmari Mannsåker]

ilm...@ilmari.org (Dagfinn Ilmari Mannsåker) writes:

> Tom Lane  writes:
>
>> Cool, I'll go commit a fix along those lines.  Thanks for tracing
>> this down!
>
> Here's one final style cleanup for the TAP test.
>
> - use like() for the banner test
> - pass the regexes around as qr// objects, so they can be
>   syntax-highlighted properly, and don't need regex
>   metacharacter-escaping backslashes doubled.
> - include the regex that didn't match in the diagnostic

This time with the actual attachment...

- ilmari
-- 
- Twitter seems more influential [than blogs] in the 'gets reported in
  the mainstream press' sense at least.   - Matt McLeod
- That'd be because the content of a tweet is easier to condense down
  to a mainstream media article.  - Calle Dybedahl
>From 0307bdea0f95e47e9ed7cf9678c12d568006d772 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= 
Date: Sun, 5 Jan 2020 13:20:10 +
Subject: [PATCH] Use qr// for passed-in regexes in tab-completion TAP test

This lets editors syntax-highlight them as regexes, not just plain
strings, and avoids having to double backslashes when escaping regex
metacharacters like *.

Also include the pattern that didn't match in the failure diagnostic,
and use like() for the startup banner test.
---
 src/bin/psql/t/010_tab_completion.pl | 25 +
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/src/bin/psql/t/010_tab_completion.pl b/src/bin/psql/t/010_tab_completion.pl
index 9cfd7ec79c..bff954de24 100644
--- a/src/bin/psql/t/010_tab_completion.pl
+++ b/src/bin/psql/t/010_tab_completion.pl
@@ -58,7 +58,7 @@ my $timer = timer(5);
 
 my $h = $node->interactive_psql('postgres', \$in, \$out, $timer);
 
-ok($out =~ /psql/, "print startup banner");
+like($out, qr/psql/, "print startup banner");
 
 # Simple test case: type something and see if psql responds as expected
 sub check_completion
@@ -75,13 +75,14 @@ sub check_completion
 	# send the data to be sent
 	$in .= $send;
 	# wait ...
-	pump $h until ($out =~ m/$pattern/ || $timer->is_expired);
-	my $okay = ($out =~ m/$pattern/ && !$timer->is_expired);
+	pump $h until ($out =~ $pattern || $timer->is_expired);
+	my $okay = ($out =~ $pattern && !$timer->is_expired);
 	ok($okay, $annotation);
 	# for debugging, log actual output if it didn't match
 	local $Data::Dumper::Terse = 1;
 	local $Data::Dumper::Useqq = 1;
-	diag 'Actual output was ' . Dumper($out) . "\n" if !$okay;
+	diag 'Actual output was ' . Dumper($out) .
+		"Did not match $pattern\n" if !$okay;
 	return;
 }
 
@@ -89,20 +90,20 @@ sub check_completion
 # (won't work if we are inside a string literal!)
 sub clear_query
 {
-	check_completion("\\r\n", "postgres=# ", "\\r works");
+	check_completion("\\r\n", qr/postgres=# /, "\\r works");
 	return;
 }
 
 # check basic command completion: SEL produces SELECT
-check_completion("SEL\t", "SELECT ", "complete SEL to SELECT");
+check_completion("SEL\t", qr/SELECT /, "complete SEL to SELECT");
 
 clear_query();
 
 # check case variation is honored
-check_completion("sel\t", "select ", "complete sel to select");
+check_completion("sel\t", qr/select /, "complete sel to select");
 
 # check basic table name completion
-check_completion("* from t\t", "\\* from tab1 ", "complete t to tab1");
+check_completion("* from t\t", qr/\* from tab1 /, "complete t to tab1");
 
 clear_query();
 
@@ -110,14 +111,14 @@ clear_query();
 # note: readline might print a bell before the completion
 check_completion(
 	"select * from my\t",
-	"select \\* from my\a?tab",
+	qr/select \* from my\a?tab/,
 	"complete my to mytab when there are multiple choices");
 
 # some versions of readline/libedit require two tabs here, some only need one
-check_completion("\t\t", "mytab123 +mytab246",
+check_completion("\t\t", qr/mytab123 +mytab246/,
 	"offer multiple table choices");
 
-check_completion("2\t", "246 ",
+check_completion("2\t", qr/246 /,
 	"finish completion of one of multiple table choices");
 
 clear_query();
@@ -125,7 +126,7 @@ clear_query();
 # check case-sensitive keyword replacement
 # note: various versions of readline/libedit handle backspacing
 # differently, so just check that the replacement comes out correctly
-check_completion("\\DRD\t", "drds ", "complete \\DRD to \\drds");
+check_completion("\\DRD\t", qr/\\drds /, "complete \\DRD to \\drds");
 
 clear_query();
 
-- 
2.22.0

Re: parallel vacuum options/syntax

[Tomas Vondra]

On Sun, Jan 05, 2020 at 09:17:57PM +0900, Masahiko Sawada wrote:

On Sun, Jan 5, 2020 at 7:26 PM Amit Kapila  wrote:


...

> If we want to have a vacuum option to determine parallel degree, we
> should probably have a vacuum option to disable parallelism using just a
> vacuum option. I don't think 0 is too bad, and disable_parallel seems a
> bit awkward. Maybe we could use NOPARALLEL (in addition to PARALLEL n).
> That's what Oracle does, so it's not entirely without a precedent.
>

We can go either way (using 0 for parallel to indicate disable
parallelism or by introducing a new option like NOPARALLEL).  I think
initially we can avoid introducing more options and just go with
'Parallel 0' and if we find a lot of people find it inconvenient, then
we can always introduce a new option later.


Hmm I'm confused. Specifying NOPARALLEL or PARALLEL 0 is the same as
setting max_parallel_maintenance_workers to 0, right? We normally set
max_parallel_workers_per_gather to 0 to disable parallel queries on a
query. So I think that disabling parallel vacuum by setting
max_parallel_maintenance_workers to 0 is the same concept. Regarding
proposed two options we already have storage parameter
parallel_workers and it accepts 0 but PARALLEL 0 looks like
contradicted at a glance. And NOPARALLEL is inconsistent with existing
DISABLE_XXX options and it's a bit awkward to specify like (NOPARALLEL
off).



My understanding is the motivation for new vacuum options is a claim
that m_p_m_w is not sufficient/suitable for the vacuum case. I've
expressed my doubts about this, but let's assume it's the right
solution. To me it seems a bit confusing to just fall back to m_p_m_w
when it comes to disabling the parallel vacuum.

So if we think we need an option to determine vacuum parallel degree, we
should have an option to disable parallelism too. I don't care much if
it's called DISABLE_PARALLEL, NOPARALLEL or PARALLEL 0, as long as we
make our mind and don't unnecessarily break it in the next release.


regards

--
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: pgsql: Add basic TAP tests for psql's tab-completion logic.

[Dagfinn Ilmari Mannsåker]

Tom Lane  writes:

> Cool, I'll go commit a fix along those lines.  Thanks for tracing
> this down!

Here's one final style cleanup for the TAP test.

- use like() for the banner test
- pass the regexes around as qr// objects, so they can be
  syntax-highlighted properly, and don't need regex
  metacharacter-escaping backslashes doubled.
- include the regex that didn't match in the diagnostic

- ilmari
-- 
"The surreality of the universe tends towards a maximum" -- Skud's Law
"Never formulate a law or axiom that you're not prepared to live with
 the consequences of."  -- Skud's Meta-Law

Re: WIP: System Versioned Temporal Table

[Vik Fearing]

On 05/01/2020 11:16, Surafel Temesgen wrote:
>
>
> On Fri, Jan 3, 2020 at 4:22 PM Vik Fearing
> mailto:vik.fear...@2ndquadrant.com>> wrote:
>
> >
> > Rebased and conflict resolved i hope it build clean this time
> >
>
> It does but you haven't included your tests file so `make check`
> fails.
>
>
>
> what tests file?


Exactly.


> i add system_versioned_table.sql and system_versioned_table.out
> test files


Those are not included in the patch.





Okay, that was user error on my side.  I apologize.


>  
>
> It seems clear to me that you haven't tested it at all anyway.  The
> temporal conditions do not return the correct results, and the
> syntax is
> wrong, too.  Also, none of my previous comments have been addressed
> except for "system versioning" instead of "system_versioning".  Why?
>
>
> I also correct typo and add row end column time to unique
> key that make it unique for current data. As you mentioned
> other comment is concerning about application-time periods
> which the patch not addressing .


- For performance, you must put the start column in the indexes also.

- You only handle timestamp when you should also handle timestamptz and
date.

- You don't throw 2201H for anomalies


> i refer sql 2011 standard for
> syntax can you tell me which syntax you find it wrong?


Okay, now that I see your tests, I understand why everything is broken. 
You only test FROM-TO and with a really wide interval.  There are no
tests for AS OF and no tests for BETWEEN-AND.


As for the syntax, you have:


select a from for stest0 system_time from '2000-01-01 00:00:00.0' to
'infinity' ORDER BY a;


when you should have:


select a from stest0 for system_time from '2000-01-01 00:00:00.0' to
'infinity' ORDER BY a;


That is, the FOR should be on the other side of the table name.


In addition, there are many rules in the standard that are not respected
here.  For example, this query works and should not:


CREATE TABLE t (system_time integer) WITH SYSTEM VERSIONING;


This syntax is not supported:


ALTER TABLE t
    ADD PERIOD FOR SYSTEM_TIME (s, e)
    ADD COLUMN s timestamp
    ADD COLUMN e timestamp;


psql's \d does not show that the table is system versioned, and doesn't
show the columns of the system_time period.


I can drop columns used in the period.


Please don't hesitate to take inspiration from my extension that does
this.  The extension is under the PostgreSQL license for that reason. 
Take from it whatever you need.

https://github.com/xocolatl/periods/

-- 

Vik Fearing

Re: parallel vacuum options/syntax

[Masahiko Sawada]

On Sun, Jan 5, 2020 at 7:26 PM Amit Kapila  wrote:
>
> On Sun, Jan 5, 2020 at 6:40 AM Tomas Vondra
>  wrote:
> >
> > On Sun, Jan 05, 2020 at 08:54:15AM +0900, Masahiko Sawada wrote:
> > >On Thu, Jan 2, 2020 at 9:09 PM Amit Kapila 
> > >wrote:
> > >>
> > >> Hi,
> > >>
> > >> I am starting a new thread for some of the decisions for a parallel
> > >> vacuum in the hope to get feedback from more people.  There are
> > >> mainly two points for which we need some feedback.
> > >>
> > >> 1. Tomas Vondra has pointed out on the main thread [1] that by
> > >> default the parallel vacuum should be enabled similar to what we do
> > >> for Create Index.  As proposed, the patch enables it only when the
> > >> user specifies it (ex. Vacuum (Parallel 2) ;).   One of the
> > >> arguments in favor of enabling it by default as mentioned by Tomas is
> > >> "It's pretty much the same thing we did with vacuum throttling - it's
> > >> disabled for explicit vacuum by default, but you can enable it. If
> > >> you're worried about VACUUM causing issues, you should set cost
> > >> delay.".  Some of the arguments against enabling it are that it will
> > >> lead to use of more resources (like CPU, I/O) which users might or
> > >> might like.
> > >>
> > >
> > >I'm a bit wary of making parallel vacuum enabled by default. Single
> > >process vacuum does sequential reads/writes on most of indexes but
> > >parallel vacuum does random access random reads/writes. I've tested
> > >parallel vacuum on HDD and confirmed the performance is good but I'm
> > >concerned that it might be cause of more disk I/O than user expected.
> > >
> >
> > I understand the concern, but it's not clear to me why to apply this
> > defensive approach just to vacuum and not to all commands. Especially
> > when we do have a way to throttle vacuum (unlike pretty much any other
> > command) if I/O really is a scarce resource.
> >
> > As the vacuum workers are separate processes, each generating requests
> > with a sequential pattern, so I'd expect readahead to kick in and keep
> > the efficiency of sequential access pattern.
> >
>
> Right, I also think so.

Okay I understand.

>
> > >> Now, if we want to enable it by default, we need a way to disable it
> > >> as well and along with that, we need a way for users to specify a
> > >> parallel degree.  I have mentioned a few reasons why we need a
> > >> parallel degree for this operation in the email [2] on the main
> > >> thread.
> > >>
> > >> If parallel vacuum is *not* enabled by default, then I think the
> > >> current way to enable is fine which is as follows: Vacuum (Parallel
> > >> 2) ;
> > >>
> > >> Here, if the user doesn't specify parallel_degree, then we internally
> > >> decide based on number of indexes that support a parallel vacuum with
> > >> a maximum of max_parallel_maintenance_workers.
> > >>
> > >> If the parallel vacuum is enabled by default, then I could think of
> > >> the following ways:
> > >>
> > >> (a) Vacuum (disable_parallel) ;  Vacuum (Parallel
> > >> ) ;
> > >>
> > >> (b) Vacuum (Parallel ) ;  If user
> > >> specifies parallel_degree as 0, then disable parallelism.
> > >>
> > >> (c) ... Any better ideas?
> > >>
> > >
> > >If parallel vacuum is enabled by default, I would prefer (b) but I
> > >don't think it's a good idea to accept 0 as parallel degree. If we want
> > >to disable parallel vacuum we should max_parallel_maintenance_workers
> > >to 0 instead.
> > >
> >
> > IMO that just makes the interaction between vacuum options and the GUC
> > even more complicated/confusing.
> >
>
> Yeah, I am also not sure if that will be a good idea.
>
> > If we want to have a vacuum option to determine parallel degree, we
> > should probably have a vacuum option to disable parallelism using just a
> > vacuum option. I don't think 0 is too bad, and disable_parallel seems a
> > bit awkward. Maybe we could use NOPARALLEL (in addition to PARALLEL n).
> > That's what Oracle does, so it's not entirely without a precedent.
> >
>
> We can go either way (using 0 for parallel to indicate disable
> parallelism or by introducing a new option like NOPARALLEL).  I think
> initially we can avoid introducing more options and just go with
> 'Parallel 0' and if we find a lot of people find it inconvenient, then
> we can always introduce a new option later.

Hmm I'm confused. Specifying NOPARALLEL or PARALLEL 0 is the same as
setting max_parallel_maintenance_workers to 0, right? We normally set
max_parallel_workers_per_gather to 0 to disable parallel queries on a
query. So I think that disabling parallel vacuum by setting
max_parallel_maintenance_workers to 0 is the same concept. Regarding
proposed two options we already have storage parameter
parallel_workers and it accepts 0 but PARALLEL 0 looks like
contradicted at a glance. And NOPARALLEL is inconsistent with existing
DISABLE_XXX options and it's a bit awkward to specify like (NOPARALLEL
off).

Regards,

--
Masahiko Sawada  http://www.2ndQuadrant.com/
PostgreSQL Developme

Re: parallel vacuum options/syntax

[Tomas Vondra]

On Sun, Jan 05, 2020 at 03:56:35PM +0530, Amit Kapila wrote:


...

>If parallel vacuum is enabled by default, I would prefer (b) but I
>don't think it's a good idea to accept 0 as parallel degree. If we want
>to disable parallel vacuum we should max_parallel_maintenance_workers
>to 0 instead.
>

IMO that just makes the interaction between vacuum options and the GUC
even more complicated/confusing.



Yeah, I am also not sure if that will be a good idea.


If we want to have a vacuum option to determine parallel degree, we
should probably have a vacuum option to disable parallelism using just a
vacuum option. I don't think 0 is too bad, and disable_parallel seems a
bit awkward. Maybe we could use NOPARALLEL (in addition to PARALLEL n).
That's what Oracle does, so it's not entirely without a precedent.



We can go either way (using 0 for parallel to indicate disable
parallelism or by introducing a new option like NOPARALLEL).  I think
initially we can avoid introducing more options and just go with
'Parallel 0' and if we find a lot of people find it inconvenient, then
we can always introduce a new option later.



I don't think starting with "parallel 0" and then maybe introducing
NOPARALLEL sometime in the future is a good plan, because after adding
NOPARALLEL we'd either have to remove "parallel 0" (breaking backwards
compatibility unnecessarily) or supporting both approaches.

regards

--
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Patch to document base64 encoding

[Fabien COELHO]

Hello Karl,


Attached is doc_base64_v11.patch


Patch applies cleanly and compiles.

I'm in favor of moving and reorganizing these function descriptions, as 
they are somehow scattered with a unclear logic when you are looking for 
them.


 +   bytea ||
 +bytea
  bytea 
 
  String concatenation

Bytea concatenation?

I'm not keen on calling the parameter the name of its type. I'd suggest to 
keep "string" as a name everywhere, which is not a type name in Pg.


The functions descriptions are not homogeneous. Some have parameter name & 
type "btrim(string bytea, bytes bytea)" and others only type or parameter 
with tagged as a parameter "get_bit(bytea, offset)" (first param), 
"sha224(bytea)".


I'd suggest to be consistent, eg use "string bytea" everywhere 
appropriate.


--
Fabien.

Re: parallel vacuum options/syntax

[Amit Kapila]

On Sun, Jan 5, 2020 at 6:40 AM Tomas Vondra
 wrote:
>
> On Sun, Jan 05, 2020 at 08:54:15AM +0900, Masahiko Sawada wrote:
> >On Thu, Jan 2, 2020 at 9:09 PM Amit Kapila 
> >wrote:
> >>
> >> Hi,
> >>
> >> I am starting a new thread for some of the decisions for a parallel
> >> vacuum in the hope to get feedback from more people.  There are
> >> mainly two points for which we need some feedback.
> >>
> >> 1. Tomas Vondra has pointed out on the main thread [1] that by
> >> default the parallel vacuum should be enabled similar to what we do
> >> for Create Index.  As proposed, the patch enables it only when the
> >> user specifies it (ex. Vacuum (Parallel 2) ;).   One of the
> >> arguments in favor of enabling it by default as mentioned by Tomas is
> >> "It's pretty much the same thing we did with vacuum throttling - it's
> >> disabled for explicit vacuum by default, but you can enable it. If
> >> you're worried about VACUUM causing issues, you should set cost
> >> delay.".  Some of the arguments against enabling it are that it will
> >> lead to use of more resources (like CPU, I/O) which users might or
> >> might like.
> >>
> >
> >I'm a bit wary of making parallel vacuum enabled by default. Single
> >process vacuum does sequential reads/writes on most of indexes but
> >parallel vacuum does random access random reads/writes. I've tested
> >parallel vacuum on HDD and confirmed the performance is good but I'm
> >concerned that it might be cause of more disk I/O than user expected.
> >
>
> I understand the concern, but it's not clear to me why to apply this
> defensive approach just to vacuum and not to all commands. Especially
> when we do have a way to throttle vacuum (unlike pretty much any other
> command) if I/O really is a scarce resource.
>
> As the vacuum workers are separate processes, each generating requests
> with a sequential pattern, so I'd expect readahead to kick in and keep
> the efficiency of sequential access pattern.
>

Right, I also think so.

> >> Now, if we want to enable it by default, we need a way to disable it
> >> as well and along with that, we need a way for users to specify a
> >> parallel degree.  I have mentioned a few reasons why we need a
> >> parallel degree for this operation in the email [2] on the main
> >> thread.
> >>
> >> If parallel vacuum is *not* enabled by default, then I think the
> >> current way to enable is fine which is as follows: Vacuum (Parallel
> >> 2) ;
> >>
> >> Here, if the user doesn't specify parallel_degree, then we internally
> >> decide based on number of indexes that support a parallel vacuum with
> >> a maximum of max_parallel_maintenance_workers.
> >>
> >> If the parallel vacuum is enabled by default, then I could think of
> >> the following ways:
> >>
> >> (a) Vacuum (disable_parallel) ;  Vacuum (Parallel
> >> ) ;
> >>
> >> (b) Vacuum (Parallel ) ;  If user
> >> specifies parallel_degree as 0, then disable parallelism.
> >>
> >> (c) ... Any better ideas?
> >>
> >
> >If parallel vacuum is enabled by default, I would prefer (b) but I
> >don't think it's a good idea to accept 0 as parallel degree. If we want
> >to disable parallel vacuum we should max_parallel_maintenance_workers
> >to 0 instead.
> >
>
> IMO that just makes the interaction between vacuum options and the GUC
> even more complicated/confusing.
>

Yeah, I am also not sure if that will be a good idea.

> If we want to have a vacuum option to determine parallel degree, we
> should probably have a vacuum option to disable parallelism using just a
> vacuum option. I don't think 0 is too bad, and disable_parallel seems a
> bit awkward. Maybe we could use NOPARALLEL (in addition to PARALLEL n).
> That's what Oracle does, so it's not entirely without a precedent.
>

We can go either way (using 0 for parallel to indicate disable
parallelism or by introducing a new option like NOPARALLEL).  I think
initially we can avoid introducing more options and just go with
'Parallel 0' and if we find a lot of people find it inconvenient, then
we can always introduce a new option later.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Re: WIP: System Versioned Temporal Table

[Surafel Temesgen]

On Mon, Oct 28, 2019 at 6:36 PM Vik Fearing 
wrote:

> On 28/10/2019 13:48, Surafel Temesgen wrote:
> >
> >
> > On Fri, Oct 25, 2019 at 10:45 PM Vik Fearing
> > mailto:vik.fear...@2ndquadrant.com>>
> wrote:
> >
> > >
> > > I don't understand what you mean by this.
> > >
> > >
> > >
> > > The primary purpose of adding row end time to primary key is to
> > allow
> > > duplicate value to be inserted into a table with keeping
> > constraint in
> > > current data but it can be duplicated in history data. Adding row
> > > start time column to primary key will eliminate this uniqueness for
> > > current data which is not correct
> >
> >
> > How?  The primary/unique keys must always be unique at every point
> > in time.
> >
> >
> > From user prospect it is acceptable to delete and reinsert a record
> > with the same key value multiple time which means there will be
> > multiple record with the same key value in a history data but there is
> > only one values in current data as a table without system versioning
> > do .I add row end time column to primary key to allow user supplied
> > primary key values to be duplicated in history data which is acceptable
> >
>
> Yes, I understand that.  I'm saying you should also add the row start
> time column.
>
>
that allow the same primary key value row to be insert as long
as insertion time is different

regards
Surafel

Re: WIP: System Versioned Temporal Table

[Surafel Temesgen]

On Fri, Jan 3, 2020 at 4:22 PM Vik Fearing 
wrote:

> >
> > Rebased and conflict resolved i hope it build clean this time
> >
>
> It does but you haven't included your tests file so `make check` fails.
>
>
>
what tests file? i add system_versioned_table.sql and
system_versioned_table.out
test files and it tested and pass on appveyor[1] only failed on travis
because of warning. i will add more test


> It seems clear to me that you haven't tested it at all anyway.  The
> temporal conditions do not return the correct results, and the syntax is
> wrong, too.  Also, none of my previous comments have been addressed
> except for "system versioning" instead of "system_versioning".  Why?
>
>
I also correct typo and add row end column time to unique
key that make it unique for current data. As you mentioned
other comment is concerning about application-time periods
which the patch not addressing . i refer sql 2011 standard for
syntax can you tell me which syntax you find it wrong?
[1].
https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.73247

regards
Surafel

Re: pgbench - add pseudo-random permutation function

[Fabien COELHO]

This patch was marked as RFC on 2019-03-30, but since then there have 
been a couple more issues pointed out in a review by Thomas Munro, and 
it went through 2019-09 and 2019-11 without any attention. Is the RFC 
status still appropriate?


Thomas review was about comments/documentation wording and asking for 
explanations, which I think I addressed, and the code did not actually 
change, so I'm not sure that the "needs review" is really needed, but do 
as you feel.



--
Fabien

Re: Planning counters in pg_stat_statements (using pgss_store)

[Julien Rouhaud]

On Fri, Nov 22, 2019 at 11:23 AM imai.yoshik...@fujitsu.com
 wrote:
>
> On Wed, Nov 20, 2019 at 4:55 PM, Julien Rouhaud wrote:
> > On Wed, Nov 20, 2019 at 2:06 AM imai.yoshik...@fujitsu.com 
> >  wrote:
> > >
> > > On Tue, Nov 19, 2019 at 2:27 PM, Julien Rouhaud wrote:
> > > > On Fri, Nov 15, 2019 at 2:00 AM imai.yoshik...@fujitsu.com 
> > > >  wrote:
> > > > >
> > > > > Actually I also don't have strong opinion but I thought someone
> > > > > would complain about renaming of those columns and
> > > > also some tools like monitoring which use those columns will not
> > > > work. If we use {total, min, max, mean, stddev}_time, someone might
> > > > mistakenly understand {total, min, max, mean, stddev}_time mean {total, 
> > > > min, max, mean, stddev} of planning and
> > execution.
> > > > > If I need to choose {total, min, max, mean, stddev}_time or
> > > > > {total, min, max, mean, stddev}_exec_time, I choose former
> > > > one because choosing best name is not worth destructing the existing 
> > > > scripts or tools.
> > > >
> > > > We could definitely keep (plan|exec)_time for the SRF, and have the
> > > > {total, min, max, mean, stddev}_time created by the view to be a sum
> > > > of planning + execution for each counter
> > >
> > > I might misunderstand but if we define {total, min, max, mean,
> > > stddev}_time is just a sum of planning + execution for each counter
> > > like "select total_plan_time + total_exec_time as total_time from
> > > pg_stat_statements", I wonder we can calculate stddev_time correctly.
> > > If we prepare variables in the codes to calculate those values, yes,
> > > we can correctly calculate those values even for the total_stddev.
> >
> > Yes you're right, this can't possibly work for most of the counters.
> > And also, since there's no guarantee that each execution will follow a 
> > planning, providing such global counters for
> > min/max/mean and stddev wouldn't make much sense.
>
> Ah, I see. Planning counts and execution counts differ.
> It might be difficult to redefine the meaning of {min, max, mean, 
> stddev}_time precisely, and even if we can redefine it correctly, it would 
> not be intuitive.

Thomas' automatic patch tester just warned me that the patchset is
broken since 3fd40b628c7db4, which removed the queryString from
ExecCreateTableAs.  New patch version that re-add the queryString, no
changes otherwise.


0001-Pass-query-string-to-the-planner-v3.patch
Description: Binary data


0002-Add-planning-counters-to-pg_stat_statements-v3.patch
Description: Binary data