Re: Evaluate expression at planning time for two more cases

[Ashutosh Bapat]

Hi Surafel,

On Thu, Aug 27, 2020 at 6:01 PM Surafel Temesgen  wrote:
>
> Hi,
>
> In good written query IS NULL and IS NOT NULL check on primary and non null 
> constraints columns should not happen but if it is mentioned PostgreSQL have 
> to be smart enough for not checking every return result about null value on 
> primary key column. Instead it can be evaluate its truth value and set the 
> result only once. The attached patch evaluate and set the truth value for 
> null and not null check on primary column on planning time if the relation 
> attribute is not mention on nullable side of outer join.
>
> Thought?

Thanks for the patch. Such SQL may arise from not-so-smart SQL
generation tools. It will be useful to have this optimization. Here
are some comments on your patch.

 }
 else
 has_nonconst_input = true;
@@ -3382,7 +3395,47 @@ eval_const_expressions_mutator(Node *node,

+
+if (pkattnos != NULL &&
bms_is_member(var->varattno - FirstLowInvalidHeapAttributeNumber,
pkattnos)
+&& !check_null_side(context->root, relid))

Since this is working on parse->rtable this will work only for top level tables
as against the inherited tables or partitions which may have their own primary
key constraints if the parent doesn't have those.

This better be done when planning individual relations, plain or join or upper,
where all the necessary information is already available with each of the
relations and also the quals, derived as well as user specified, are
distributed to individual relations where they should be evalutated. My memory
is hazy but it might be possible do this while distributing the quals
themselves (distribute_qual_to_rels()).

Said that, to me, this looks more like something we should be able to do at the
time of constraint exclusion. But IIRC, we just prove whether constraints
refute a qual and not necessarily whether constraints imply a qual, making it
redundant, as is required here. E.g. primary key constraint implies key NOT
NULL rendering a "key IS NOT NULL" qual redundant. It might be better to test
the case when col IS NOT NULL is specified on a column which already has a NOT
NULL constraint. That may be another direction to take. We may require much
lesser code.

With either of these two approaches, the amount of code changes might
be justified.

+explain (costs off)
+SELECT * FROM b RIGHT JOIN a ON (b.a_id = a.id) WHERE (a.id IS NULL
OR a.id > 0);
+  QUERY PLAN
+---
+ Hash Right Join
+   Hash Cond: (b.a_id = a.id)
+   ->  Seq Scan on b
+   ->  Hash
+ ->  Bitmap Heap Scan on a
+   Recheck Cond: (id > 0)
+   ->  Bitmap Index Scan on a_pkey
+ Index Cond: (id > 0)
+(8 rows)

Thanks for the tests.

Please add the patch to the next commitfest https://commitfest.postgresql.org/.

-- 
Best Wishes,
Ashutosh Bapat

Re: passwordcheck: Log cracklib diagnostics

[Peter Eisentraut]

On 2020-08-25 15:32, Laurenz Albe wrote:

On Tue, 2020-08-25 at 13:48 +0200, Daniel Gustafsson wrote:

On 25 Aug 2020, at 12:20, Peter Eisentraut  
wrote:

A user tried to use the cracklib build-time option of the passwordcheck module. 
 This failed, as it turned out because there was no dictionary installed in the 
right place, but the error was not
properly reported, because the existing code just throws away the error message 
from cracklib.  Attached is a patch that changes this by logging any error 
message returned from the cracklib call.


+1 on this, it's also in line with the example documentation from cracklib.
The returned error is potentially a bit misleading now, as it might say claim
that a strong password is easily cracked if the dictionary fails load.  Given
that there is no way to distinguish between the class of returned errors it's
hard to see how we can do better though.

While poking at this, we might as well update the docs to point to the right
URL for CrackLib as it moved from Sourceforge five years ago.  The attached
diff fixes that.


+1 on both patches.


Pushed both patches, thanks.

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

Re: poc - possibility to write window function in PL languages

[Pavel Stehule]

st 26. 8. 2020 v 17:06 odesílatel Pavel Stehule 
napsal:

> Hi
>
> I simplified access to results of  winfuncargs functions by proxy type
> "typedvalue". This type can hold any Datum value, and allows fast cast to
> basic buildin types or it can use (slower) generic cast functions. It is
> used in cooperation with a plpgsql assign statement that can choose the
> correct cast implicitly. When the winfuncarg function returns a value of
> the same type, that is expected by the variable on the left side of the
> assign statement, then (for basic types), the value is just copied without
> casts. With this proxy type is not necessary to have special statement for
> assigning returned value from winfuncargs functions, so source code of
> window function in plpgsql looks intuitive to me.
>
> Example - implementation of "lag" function in plpgsql
>
> create or replace function pl_lag(numeric)
> returns numeric as $$
> declare v numeric;
> begin
>   v := get_input_value_in_partition(windowobject, 1, -1, 'seek_current',
> false);
>   return v;
> end;
> $$ language plpgsql window;
>
> I think this code is usable, and I assign this patch to commitfest.
>
> Regards
>
> Pavel
>

fix regress tests and some doc
diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml
index 815912666d..5b4d5bbac4 100644
--- a/doc/src/sgml/plpgsql.sgml
+++ b/doc/src/sgml/plpgsql.sgml
@@ -4568,6 +4568,99 @@ CREATE EVENT TRIGGER snitch ON ddl_command_start EXECUTE FUNCTION snitch();
 
   
 
+ 
+  Window Functions
+
+  
+   window
+   in PL/pgSQL
+  
+
+  
+   PL/pgSQL can be used to define window
+   functions. A window function is created with the CREATE FUNCTION
+   command with clause WINDOW. The specific feature of
+   this functions is a possibility to two special storages with 
+   sorted values of window function arguments and store with stored
+   one value of any type for currently processed partition (of window
+   function).
+  
+
+  
+   Access to both storages is done with special internal variable
+   WINDOWOBJECT. This variable is declared implicitly,
+   and it is available only in window functions.
+
+
+CREATE OR REPLACE FUNCTION plpgsql_rownum() RETURNS int8
+LANGUAGE plpgsql WINDOW
+AS $$
+DECLARE pos int8
+BEGIN
+pos := get_current_position(WINDOWOBJECT);
+pos := pos + 1;
+PERFORM set_mark_position(WINDOWOBJECT, pos);
+RETURN pos;
+$$;
+
+SELECT plpgsql_rownum() OVER (), * FROM tab;
+
+  
+
+  
+   The arguments of window function cannot be accessed directly. The special
+   functions should be used. With these functions we can choose a scope of
+   buffered arguments, we can choose a wanted position against first, current, or
+   last row. The implementation of lag can looks like
+   (the window functions in plpgsql can use polymorphic types too):
+
+
+CREATE OR REPLACE FUNCTION plpgsql_lag(anyelement) RETURNS anyelement
+LANGUAGE plpgsql WINDOW
+AS $$
+BEGIN
+RETURN
+  get_input_value_in_partition(WINDOWOBJECT,
+   1, -1,
+   'seek_current',
+   false);
+END;
+$$;
+
+SELECT v, plpgsql_lag(v) FROM generate_series(1, 10) g(v);
+
+
+  
+
+  
+   Second buffer that can be used in window function is a buffer for one value
+   assigned to partition. The content of this buffer can be read by function
+   get_partition_context_value or modified by function
+   set_partition_context_value. Next function replaces
+   missing values by previous non NULL value:
+
+
+CREATE OR REPLACE FUNCTION plpgsql_replace_missing(numeric) RETURNS numeric
+LANGUAGE plpgsql WINDOW
+AS $$
+DECLATE
+v numeric;
+BEGIN
+v := get_input_value_for_row(WINDOWOBJECT, 1);
+IF v IS NULL THEN
+v := get_partition_context_value(WINDOWOBJECT, NULL::numeric);
+ELSE
+PERFORM set_partition_context_value(WINDOWOBJECT, v);
+END IF;
+RETURN v;
+END;
+$$;
+
+
+  
+
+  
+
   
PL/pgSQL under the Hood
 
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index a2d61302f9..f926f2e386 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -1421,6 +1421,18 @@ LANGUAGE internal
 STRICT IMMUTABLE PARALLEL SAFE
 AS 'unicode_is_normalized';
 
+CREATE OR REPLACE FUNCTION
+  get_partition_context_value(windowobjectproxy, anyelement, int4 DEFAULT NULL)
+  RETURNS anyelement
+LANGUAGE internal
+AS 'windowobject_get_partition_context_value';
+
+CREATE OR REPLACE FUNCTION
+  set_partition_context_value(windowobjectproxy, anyelement, int4 DEFAULT NULL)
+  RETURNS void
+LANGUAGE internal
+AS 'windowobject_set_partition_context_value';
+
 --
 -- The default permissions for functions mean that anyone can execute them.
 -- A number of functions shouldn't be executable by just anyone, but rather
diff --git a/src/backend/utils/adt/Makefile b/src/backend/utils/adt/Makefile
index 54d5c37947..84da7222d9 100644
--- a/src/backend/utils/adt/Makefile
+++ b/s

Re: Clang UndefinedBehaviorSanitize (Postgres14) Detected undefined-behavior

[Noah Misch]

On Thu, Aug 27, 2020 at 11:11:47PM -0400, Tom Lane wrote:
> Noah Misch  writes:
> > On Thu, Aug 27, 2020 at 12:57:20PM -0400, Alvaro Herrera wrote:
> >> Looks legit, and at least per commit 13bba02271dc we do fix such things,
> >> even if it's useless in practice.
> >> Given that no buildfarm member has ever complained, this exercise seems
> >> pretty pointless.
> 
> > Later decision to stop changing such code:
> > https://postgr.es/m/flat/e1a26ece-7057-a234-d87e-4ce1cdc9e...@2ndquadrant.com

> I think that the actual risk here has to do not with
> what memcmp() might do, but with what gcc might do in code surrounding
> the call, once it's armed with the assumption that any pointer we pass
> to memcmp() could not be null.  See
> 
> https://www.postgresql.org/message-id/flat/BLU437-SMTP48A5B2099E7134AC6BE7C7F2A10%40phx.gbl
> 
> It's surely not hard to visualize cases where necessary code could
> be optimized away if the compiler thinks it's entitled to assume
> such things.

Good point.  We could pick from a few levels of concern:

- No concern: reject changes serving only to remove this class of deviation.
  This is today's policy.
- Medium concern: accept fixes, but the buildfarm continues not to break in
  the face of new deviations.  This will make some code uglier, but we'll be
  ready against some compiler growing the optimization you describe.
- High concern: I remove -fno-sanitize=nonnull-attribute from buildfarm member
  thorntail.  In addition to the drawback of the previous level, this will
  create urgent work for committers introducing new deviations (or introducing
  test coverage that unearths old deviations).  This is our current response
  to Valgrind complaints, for example.

Re: Support for OUT parameters in procedures

[Peter Eisentraut]

On 2020-08-27 15:56, Robert Haas wrote:

On Thu, Aug 27, 2020 at 4:34 AM Peter Eisentraut
 wrote:

For a top-level direct call, you can pass whatever you want, since all
OUT parameters are presented as initially NULL to the procedure code.
So you could just pass NULL, as in CALL test_proc(5, NULL).


Is that actually how other systems work? I would think that people
would expect to pass, say, a package variable, and expect that it will
get updated.


The handling of results of SQL statements executed at the top level 
(a.k.a. direct SQL) is implementation-specific and varies widely in 
practice.  More interesting in practice, in terms of functionality and 
also compatibility, are nested calls in PL/pgSQL as well as integration 
in JDBC.


We already support INOUT parameters in procedures, so the method of 
returning the value of output parameters after the CALL already exists. 
This patch doesn't touch that at all, really.  If we had or would add 
other places to put those results, such as package variables, then they 
could be added independently of this patch.


Of course, feedback from those more knowledgeable in other systems than 
me would be welcome.


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

Re: Handing off SLRU fsyncs to the checkpointer

[Thomas Munro]

On Thu, Aug 27, 2020 at 8:48 PM Jakub Wartak  wrote:
> >> 29.62%  postgres  [kernel.kallsyms]   [k] 
> >> copy_user_enhanced_fast_string
> >> ---copy_user_enhanced_fast_string
> >>|--17.98%--copyin
> >> [..]
> >>|  __pwrite_nocancel
> >>|  FileWrite
> >>|  mdwrite
> >>|  FlushBuffer
> >>|  ReadBuffer_common
> >>|  ReadBufferWithoutRelcache
> >>|  XLogReadBufferExtended
> >>|  XLogReadBufferForRedoExtended
> >>|   --17.57%--btree_xlog_insert
> >
> > To move these writes out of recovery's way, we should probably just
> > run the bgwriter process during crash recovery.  I'm going to look
> > into that.
>
> Sounds awesome.

I wrote a quick and dirty experimental patch to try that.  I can't see
any benefit from it on pgbench with default shared buffers, but maybe
it would do better with your append test due to locality, especially
if you can figure out how to tune bgwriter to pace itself optimally.
https://github.com/macdice/postgres/tree/bgwriter-in-crash-recovery

Re: SQL-standard function body

[Peter Eisentraut]

On 2020-06-30 19:49, Peter Eisentraut wrote:

This adds support for writing CREATE FUNCTION and CREATE PROCEDURE
statements for language SQL with a function body that conforms to the
SQL standard and is portable to other implementations.


Here is a new patch.  The only significant change is that pg_dump 
support is now fixed.  Per the discussion in [0], I have introduced a 
new function pg_get_function_sqlbody() that just produces the formatted 
SQL body, not the whole function definition.  All the tests now pass. 
As mentioned before, more tests are probably needed, so if reviewers 
just want to play with this and find things that don't work, those could 
be put into test cases, for example.


As a thought, a couple of things could probably be separated from this 
patch and considered separately:


1. making LANGUAGE SQL the default

2. the RETURN statement

If reviewers think that would be sensible, I can prepare separate 
patches for those.



[0]: 
https://www.postgresql.org/message-id/flat/9df8a3d3-13d2-116d-26ab-6a273c1ed38c%402ndquadrant.com


--
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From 4df3c9db684cd1de6c9bfa622829308a4f8809a7 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Fri, 28 Aug 2020 07:19:10 +0200
Subject: [PATCH v2] SQL-standard function body

This adds support for writing CREATE FUNCTION and CREATE PROCEDURE
statements for language SQL with a function body that conforms to the
SQL standard and is portable to other implementations.

Instead of the PostgreSQL-specific AS $$ string literal $$ syntax,
this allows writing out the SQL statements making up the body
unquoted, either as a single statement:

CREATE FUNCTION add(a integer, b integer) RETURNS integer
LANGUAGE SQL
RETURN a + b;

or as a block

CREATE PROCEDURE insert_data(a integer, b integer)
LANGUAGE SQL
BEGIN ATOMIC
  INSERT INTO tbl VALUES (a);
  INSERT INTO tbl VALUES (b);
END;

The function body is parsed at function definition time and stored as
expression nodes in probin.  So at run time, no further parsing is
required.

However, this form does not support polymorphic arguments, because
there is no more parse analysis done at call time.

Dependencies between the function and the objects it uses are fully
tracked.

A new RETURN statement is introduced.  This can only be used inside
function bodies.  Internally, it is treated much like a SELECT
statement.

psql needs some new intelligence to keep track of function body
boundaries so that it doesn't send off statements when it sees
semicolons that are inside a function body.

Also, per SQL standard, LANGUAGE SQL is the default, so it does not
need to be specified anymore.

Discussion: 
https://www.postgresql.org/message-id/flat/1c11f1eb-f00c-43b7-799d-2d44132c0...@2ndquadrant.com
---
 doc/src/sgml/ref/create_function.sgml | 126 +++--
 doc/src/sgml/ref/create_procedure.sgml|  62 ++-
 src/backend/catalog/pg_proc.c | 148 ---
 src/backend/commands/aggregatecmds.c  |   2 +
 src/backend/commands/functioncmds.c   |  96 +++---
 src/backend/executor/functions.c  |  79 
 src/backend/nodes/copyfuncs.c |  15 ++
 src/backend/nodes/equalfuncs.c|  13 ++
 src/backend/nodes/outfuncs.c  |  12 ++
 src/backend/nodes/readfuncs.c |   1 +
 src/backend/optimizer/util/clauses.c  |  25 ++-
 src/backend/parser/analyze.c  |  35 
 src/backend/parser/gram.y | 126 ++---
 src/backend/tcop/postgres.c   |   3 +-
 src/backend/utils/adt/ruleutils.c | 110 +++-
 src/bin/pg_dump/pg_dump.c |  45 +++--
 src/fe_utils/psqlscan.l   |  23 ++-
 src/include/catalog/pg_proc.dat   |   4 +
 src/include/catalog/pg_proc.h |   2 +-
 src/include/commands/defrem.h |   2 +
 src/include/executor/functions.h  |  15 ++
 src/include/fe_utils/psqlscan_int.h   |   2 +
 src/include/nodes/nodes.h |   1 +
 src/include/nodes/parsenodes.h|  13 ++
 src/include/parser/kwlist.h   |   2 +
 src/include/tcop/tcopprot.h   |   1 +
 src/interfaces/ecpg/preproc/ecpg.addons   |   6 +
 src/interfaces/ecpg/preproc/ecpg.trailer  |   4 +-
 .../regress/expected/create_function_3.out| 170 +-
 .../regress/expected/create_procedure.out |  58 ++
 src/test/regress/sql/create_function_3.sql|  77 +++-
 src/test/regress/sql/create_procedure.sql |  26 +++
 32 files changed, 1114 insertions(+), 190 deletions(-)

diff --git a/doc/src/sgml/ref/create_function.sgml 
b/doc/src/sgml/ref/create_function.sgml
index f81cedc823..f7cc428773 100644
--- a/doc/src/sgml/ref/create

Re: new heapcheck contrib module

[Andrey M. Borodin]

> 25 авг. 2020 г., в 19:36, Mark Dilger  
> написал(а):

Hi Mark!

Thanks for working on this important feature.

I was experimenting a bit with our internal heapcheck and found out that it's 
not helping with truncated CLOG anyhow.
Will your module be able to gather tid's of similar corruptions?

server/db M # select * from heap_check('pg_toast.pg_toast_4848601');
ERROR:  58P01: could not access status of transaction 636558742
DETAIL:  Could not open file "pg_xact/025F": No such file or directory.
LOCATION:  SlruReportIOError, slru.c:913
Time: 3439.915 ms (00:03.440)

Thanks!

Best regards, Andrey Borodin.

Re: Add Information during standby recovery conflicts

[Masahiko Sawada]

On Thu, 27 Aug 2020 at 20:58, Drouvot, Bertrand  wrote:
>
>
> On 8/27/20 10:16 AM, Masahiko Sawada wrote
> >
> > On Mon, 10 Aug 2020 at 23:43, Drouvot, Bertrand  wrote:
> >> Hi,
> >>
> >> On 7/31/20 7:12 AM, Masahiko Sawada wrote:
> >>> +   tmpWaitlist = waitlist;
> >>> +   while (VirtualTransactionIdIsValid(*tmpWaitlist))
> >>> +   {
> >>> +   tmpWaitlist++;
> >>> +   }
> >>> +
> >>> +   num_waitlist_entries = (tmpWaitlist - waitlist);
> >>> +
> >>> +   /* display wal record information */
> >>> +   if (log_recovery_conflicts &&
> >>> (TimestampDifferenceExceeds(recovery_conflicts_log_time,
> >>> GetCurrentTimestamp(),
> >>> +   DeadlockTimeout))) {
> >>> +   LogBlockedWalRecordInfo(num_waitlist_entries, reason);
> >>> +   recovery_conflicts_log_time = GetCurrentTimestamp();
> >>> +   }
> >>>
> >>> recovery_conflicts_log_time is not initialized. And shouldn't we
> >>> compare the current timestamp to the timestamp when the startup
> >>> process started waiting?
> >>>
> >>> I think we should call LogBlockedWalRecordInfo() inside of the inner
> >>> while loop rather than at the beginning of
> >>> ResolveRecoveryConflictWithVirtualXIDs(). In lock conflict cases, the
> >>> startup process waits until 'ltime', then enters
> >>> ResolveRecoveryConflictWithVirtualXIDs() after reaching 'ltime'.
> >>> Therefore, it makes sense to call LogBlockedWalRecordInfo() at the
> >>> beginning of ResolveRecoveryConflictWithVirtualXIDs(). However, in
> >>> snapshot and tablespace conflict cases (i.g.
> >>> ResolveRecoveryConflictWithSnapshot() and
> >>> ResolveRecoveryConflictWithTablespace()), it enters
> >>> ResolveRecoveryConflictWithVirtualXIDs() without waits and waits for
> >>> reaching ‘ltime’ inside of the inner while look. So the above
> >>> condition could always be false.
> >> That would make the information being displayed after
> >> max_standby_streaming_delay is reached for the multiple cases you just
> >> described.
> > Sorry, it should be deadlock_timeout, not max_standby_streaming_delay.
> > Otherwise, the recovery conflict log message is printed when
> > resolution, which seems not to achieve the original purpose. Am I
> > missing something?
>
> Ok, I understand where the confusion is coming from.
>
> Indeed the new version is now printing the recovery conflict log message
> during the conflict resolution (while the initial intention was to be
> warned as soon as the replay had to wait).
>
> The advantage of the new version is that it would be consistent across
> all the conflicts scenarios (if not, we would get messages during the
> resolution or when the replay started waiting, depending of the conflict
> scenario).
>
> On the other hand, the cons of the new version is that we would miss
> messages when no resolution is needed (replay wait duration <
> max_standby_streaming_delay), but is that really annoying? (As no
> cancellation would occur)
>
> Thinking about it, i like the new version (being warned during the
> resolution) as we would get messages only when cancelation will occur
> (which is what the user might want to avoid, so the extra info would be
> useful).
>
> What do you think?

Hmm, I think we print the reason why backends are canceled even of as
now by ProcessInterrupts(). With this patch and related patches you
proposed on another thread, the startup process reports virtual xids
being interrupted, the reason, and LSN of blocked WAL, then processes
will also report its virtual xid and reason. Therefore, the new
information added by these patches is only the LSN of blocked WAL.
Also, the blocked WAL would be unblocked just after the startup
process reports the resolution message. What use cases are you
assuming?

Regards,

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

Re: Should we replace the checks for access method OID with handler OID?

[Ashutosh Sharma]

On Thu, Aug 27, 2020 at 9:21 PM Robert Haas  wrote:
>
> On Thu, Aug 27, 2020 at 5:37 AM Ashutosh Sharma  wrote:
> > While reviewing the patch for pg_surgery contrib module - [1], Asim
> > Praveen suggested that it would be better to replace the check for
> > access method OID with handler OID. Otherwise, if someone creates a
> > new AM using the AM handler that is originally supported for e.g.
> > "heap_tableam_handler" and if this new AM is used to create a table,
> > then one cannot perform surgery on such tables because we have checks
> > for access method OID which would reject this new AM as we only allow
> > heap AM. For e.g. if we do this:
> >
> > create access method myam type table handler heap_tableam_handler;
> > create table mytable (…) using myam;
> >
> > And use an access method OID check, we won't be able to perform
> > surgery on mytable created above because it isn't the heap table
> > although its table structure is actually heap.
>
> The only reason I can see why it would make sense to do this sort of
> thing is if you wanted to create a new AM for testing purposes which
> behaves like some existing AM but is technically a different AM. And
> if you did that, then I guess the change you are proposing would make
> it behave more like it's the same thing after all, which seems like it
> might be missing the point.
>

Okay, understood.

-- 
With Regards,
Ashutosh Sharma
EnterpriseDB:http://www.enterprisedb.com

Re: list of extended statistics on psql

[Alvaro Herrera]

On 2020-Aug-28, Tatsuro Yamada wrote:

> > IMO the per-type columns should show both the type being enabled as
> > well as it being built.
> 
> Hmm. I'm not sure how to get the status (enabled or disabled) of
> extended stats. :(
> Could you explain it more?

pg_statistic_ext_data.stxdndistinct is not null if the stats have been
built.  (I'm not sure whether there's an easier way to determine this.)


> * The suggested column order is like this:
> ===
>Name| Schema | Table | Columns  | Ndistinct | Dependencies | 
> MCV
> ---++---+--+---+--+-
>  stts_1| public | t1| a, b | f | t| f
>  stts_2| public | t1| a, b | t | t| f
>  stts_3| public | t1| a, b | t | t| t
>  stts_4| public | t2| b, c | t | t| t
> ===

I suggest to do this

Name| Schema | Definition   | Ndistinct | Dependencies | MCV
 
---++--+---+--+-
  stts_1| public | (a, b) FROM t1   | f | t| f

> I suppose that the current column order is sufficient if there is
> no improvement of extended stats on PG14. Do you know any plan to
> improve extended stats such as to allow it to cross multiple tables on PG14?

I suggest that changing it in the future is going to be an uphill
battle, so better get it right from the get go, without requiring a
future restructure.

> In addition,
> Currently, I use this query to get Extended stats info from pg_statistic_ext.

Maybe something like this would do

SELECT
 stxnamespace::pg_catalog.regnamespace AS "Schema",
 stxname AS "Name",
 format('%s FROM %s',
 (SELECT pg_catalog.string_agg(pg_catalog.quote_ident(attname),', ')
  FROM pg_catalog.unnest(stxkeys) s(attnum)
  JOIN pg_catalog.pg_attribute a ON (stxrelid = a.attrelid AND
  a.attnum = s.attnum AND NOT attisdropped)),
  stxrelid::regclass) AS "Definition",
  CASE WHEN stxdndistinct IS NOT NULL THEN 'built' WHEN 'd' = any(stxkind) THEN 
'enabled, not built' END AS "n-distinct",
  CASE WHEN stxddependencies IS NOT NULL THEN 'built' WHEN 'f' = any(stxkind) 
THEN 'enabled, not built' END AS "functional dependencies",
  CASE WHEN stxdmcv IS NOT NULL THEN 'built' WHEN 'm' = any(stxkind) THEN 
'enabled, not built' END AS mcv
 FROM pg_catalog.pg_statistic_ext es
 INNER JOIN pg_catalog.pg_class c
 ON stxrelid = c.oid
 LEFT JOIN pg_catalog.pg_statistic_ext_data esd ON es.oid = esd.stxoid
 ORDER BY 1, 2, 3;

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Clang UndefinedBehaviorSanitize (Postgres14) Detected undefined-behavior

[Tom Lane]

Noah Misch  writes:
> On Thu, Aug 27, 2020 at 12:57:20PM -0400, Alvaro Herrera wrote:
>> Looks legit, and at least per commit 13bba02271dc we do fix such things,
>> even if it's useless in practice.
>> Given that no buildfarm member has ever complained, this exercise seems
>> pretty pointless.

> Later decision to stop changing such code:
> https://postgr.es/m/flat/e1a26ece-7057-a234-d87e-4ce1cdc9e...@2ndquadrant.com

I agree that this seems academic for any sane implementation of memcmp
and friends.  If the function is not allowed to fetch or store any bytes
when the length parameter is zero, which it certainly is not, then how
could it matter whether the pointer parameter is NULL?  It would be
interesting to know the rationale behind the C standard's claim that
this case should be undefined.

Having said that, I think that the actual risk here has to do not with
what memcmp() might do, but with what gcc might do in code surrounding
the call, once it's armed with the assumption that any pointer we pass
to memcmp() could not be null.  See

https://www.postgresql.org/message-id/flat/BLU437-SMTP48A5B2099E7134AC6BE7C7F2A10%40phx.gbl

It's surely not hard to visualize cases where necessary code could
be optimized away if the compiler thinks it's entitled to assume
such things.

In other words, the C standard made a damfool decision and now we need
to deal with the consequences of that as perpetrated by other fools.
Still, it's all hypothetical so far --- does anyone have examples of
actual rather than theoretical issues?

regards, tom lane

Re: recovering from "found xmin ... from before relfrozenxid ..."

[Ashutosh Sharma]

Hi Mark,

Thanks for the review. Please find my comments inline below:

> HeapTupleForceOption should be added to src/tools/pgindent/typedefs.list.
>

This has been fixed in the v9 patch.

>
> The tidcmp function can be removed, and ItemPointerCompare used directly by 
> qsort as:
>
> -   qsort((void*) tids, ntids, sizeof(ItemPointerData), tidcmp);
> +   qsort((void*) tids, ntids, sizeof(ItemPointerData),
> + (int (*) (const void *, const void *)) 
> ItemPointerCompare);
>

Will have a look into this.

>
> sanity_check_tid_array() has two error messages:
>
>   "array must not contain nulls"
>   "empty tid array"
>
> I would change the first to say "tid array must not contain nulls", as "tid" 
> is the name of the parameter being checked.  It is also more consistent with 
> the second error message, but that doesn't matter to me so much, as I'd argue 
> for removing the second check.  I don't see why an empty array should draw an 
> error.  It seems more reasonable to just return early since there is no work 
> to do.  Consider if somebody uses a function that returns the tids for all 
> corrupt tuples in a table, aggregates that into an array, and hands that to 
> this function.  It doesn't seem like an error for that aggregated array to 
> have zero elements in it.  I suppose you could emit a NOTICE in this case?
>

This comment is no more valid as per the changes done in the v9 patch.

>
> Upthread:
> > On Aug 13, 2020, at 12:03 PM, Robert Haas  wrote:
> >
> >> This looks like a very good suggestion to me. I will do this change in
> >> the next version. Just wondering if we should be doing similar changes
> >> in other contrib modules (like pgrowlocks, pageinspect and
> >> pgstattuple) as well?
> >
> > It seems like it should be consistent, but I'm not sure the proposed
> > change is really an improvement.
>
> You have used Asim's proposed check:
>
> if (rel->rd_amhandler != HEAP_TABLE_AM_HANDLER_OID)
> ereport(ERROR,
> (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
>  errmsg("only the relation using heap_tableam_handler is 
> supported")));
>
> which Robert seems unenthusiastic about, but if you are going that direction, 
> I think at least the language of the error message should be changed. I 
> recommend something like:
>
> if (rel->rd_amhandler != HEAP_TABLE_AM_HANDLER_OID)
> ereport(ERROR,
> (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> -errmsg("only the relation using 
> heap_tableam_handler is supported")));
> +errmsg("\"%s\" does not use a heap access 
> method",
> +   
> RelationGetRelationName(rel;
>
> where "a heap access method" could also be written as "a heap table type 
> access method", "a heap table compatible access method", and so forth.  There 
> doesn't seem to be enough precedent to dictate exactly how to phrase this, or 
> perhaps I'm just not looking in the right place.
>

Same here. This also looks invalid as per the changes done in v9 patch.

>
> The header comment for function find_tids_one_page should state the 
> requirement that the tids array must be sorted.
>

Okay, will add a comment for this.

>
> The heap_force_common function contains multiple ereport(NOTICE,...) within a 
> critical section.  I don't think that is normal practice.  Can those reports 
> be buffered until after the critical section is exited?  You also have a 
> CHECK_FOR_INTERRUPTS within the critical section.
>

This has been fixed in the v9 patch.

> [1] https://commitfest.postgresql.org/29/2700/
> —

Thanks for adding a commitfest entry for this.

-- 
With Regards,
Ashutosh Sharma
EnterpriseDB:http://www.enterprisedb.com

Re: recovering from "found xmin ... from before relfrozenxid ..."

[Ashutosh Sharma]

On Fri, Aug 28, 2020 at 1:44 AM Robert Haas  wrote:
>
> On Wed, Aug 26, 2020 at 10:26 PM Ashutosh Sharma  
> wrote:
> > Okay, point noted.
>
> I spent some time today working on this patch. I'm fairly happy with
> it now and intend to commit it if nobody sees a big problem with that.
> Per discussion, I do not intend to back-patch at this time. The two
> most significant changes I made to your version are:
>
> 1. I changed things around to avoid using any form of ereport() in a
> critical section. I'm not actually sure whether it is project policy
> to avoid ereport(NOTICE, ...) or similar in a critical section, but it
> seems prudent, because if anything fails in a critical section, we
> will PANIC, so doing fewer things there seems prudent.
>
> 2. I changed the code so that it does not try to follow redirected
> line pointers; instead, it skips them with an appropriate message, as
> we were already doing for dead and unused line pointers. I think the
> way you had it coded might've been my suggestion originally, but the
> more I looked into it the less I liked it. One problem is that it
> didn't match the docs. A second is that following a corrupted line
> pointer might index off the end of the line pointer array, and while
> that probably shouldn't happen, we are talking about corruption
> recovery here. Then I realized that, as you coded it, if the line
> pointer was redirected to a line pointer that is in turn dead (or
> unused, if there's corruption) the user would get a NOTICE complaining
> about a TID they hadn't specified, which seems like it would be very
> confusing. I thought about trying to fix all that stuff, but it just
> didn't seem worth it, because I can't think of a good reason to pass
> this function the TID of a redirected line pointer in the first place.
> If you're doing surgery, you should probably specify the exact thing
> upon which you want to operate, not some other thing that points to
> it.
>
> Here is a list of other changes I made:
>
> * Added a .gitignore file.
> * Renamed the regression test file from pg_surgery to heap_surgery to
> match the name of the single C source file we currently have.
> * Capitalized TID in a few places.
> * Ran pgindent.
> * Adjusted various comments.
> * Removed the check for an empty TID array. I don't see any reason why
> this should be an error case and I don't see much precedent for having
> such a check.
> * Fixed the code to work properly with that change and added a test case.
> * Added a check that the array is not multi-dimensional.
> * Put the AM type check before the relkind check, following existing 
> precedent.
> * Adjusted the check to use the AM OID rather than the handler OID,
> following existing precedent. Fixed the message wording accordingly.
> * Changed the documentation wording to say less about specific
> recovery procedures and focus more on the general idea that this is
> dangerous.
> * Removed all but one of the test cases that checked what happens if
> you use this on a non-heap; three tests for basically the same thing
> seemed excessive.
> * Added some additional tests to improve code coverage. There are now
> only a handful of lines not covered.
> * Reorganized the test cases somewhat.
>
> New patch attached.
>

Thank you Robert for the patch. I've looked into the changes you've
made to the v8 patch and they all look good to me.

-- 
With Regards,
Ashutosh Sharma
EnterpriseDB:http://www.enterprisedb.com

Re: Clang UndefinedBehaviorSanitize (Postgres14) Detected undefined-behavior

[Noah Misch]

On Thu, Aug 27, 2020 at 12:57:20PM -0400, Alvaro Herrera wrote:
> On 2020-Aug-27, Ranier Vilela wrote:
> > indexcmds.c (1162):
> > memcpy(part_oids, partdesc->oids, sizeof(Oid) * nparts);
> 
> Looks legit, and at least per commit 13bba02271dc we do fix such things,
> even if it's useless in practice.
> 
> Given that no buildfarm member has ever complained, this exercise seems
> pretty pointless.

Later decision to stop changing such code:
https://postgr.es/m/flat/e1a26ece-7057-a234-d87e-4ce1cdc9e...@2ndquadrant.com

Re: list of extended statistics on psql

[Tatsuro Yamada]

Hi Alvaro!

It's been ages since we created a progress reporting feature together. :-D


+1 good idea


+1 that's a good idea.  Please add it to the next commitfest!


+1 for the general idea, and +1 for \dX being the syntax to use


Thank you for voting!



IMO the per-type columns should show both the type being enabled as

well as it being built.

Hmm. I'm not sure how to get the status (enabled or disabled) of
extended stats. :(
Could you explain it more?



Also, the stat obj name column should be first, followed by a single
column listing both table and columns that it applies to.  Keep in mind
that in the future we might want to add stats that cross multiple tables
-- that's why the CREATE syntax is the way it is.  So we should give
room for that in psql's display too.


I understand your suggestions are the following, right?

* The Current column order:
===
  Schema | Table |  Name  | Columns | Ndistinct | Dependencies | MCV
+---++-+---+--+-
  public | t1| stts_1 | a, b| f | t| f
  public | t1| stts_2 | a, b| t | t| f
  public | t1| stts_3 | a, b| t | t| t
  public | t2| stts_4 | b, c| t | t| t
===

* The suggested column order is like this:
===
   Name| Schema | Table | Columns  | Ndistinct | Dependencies | MCV
---++---+--+---+--+-
 stts_1| public | t1| a, b | f | t| f
 stts_2| public | t1| a, b | t | t| f
 stts_3| public | t1| a, b | t | t| t
 stts_4| public | t2| b, c | t | t| t
===

*  In the future, Extended stats that cross multiple tables will be
   shown maybe... (t1, t2):
===
   Name| Schema | Table  | Columns  | Ndistinct | Dependencies | MCV
---+++--+---+--+-
 stts_5| public | t1, t2 | a, b | f | t| f
===

If so, I can revise the column order as you suggested easily.
However, I have no idea how to show extended stats that cross
multiple tables and the status now.

I suppose that the current column order is sufficient if there is
no improvement of extended stats on PG14. Do you know any plan to
improve extended stats such as to allow it to cross multiple tables on PG14?


In addition,
Currently, I use this query to get Extended stats info from pg_statistic_ext.

SELECT
stxnamespace::pg_catalog.regnamespace AS "Schema",
c.relname AS "Table",
stxname AS "Name",
(SELECT pg_catalog.string_agg(pg_catalog.quote_ident(attname),', ')
 FROM pg_catalog.unnest(stxkeys) s(attnum)
 JOIN pg_catalog.pg_attribute a ON (stxrelid = a.attrelid AND
 a.attnum = s.attnum AND NOT attisdropped)) AS "Columns",
'd' = any(stxkind) AS "Ndistinct",
'f' = any(stxkind) AS "Dependencies",
'm' = any(stxkind) AS "MCV"
FROM pg_catalog.pg_statistic_ext
INNER JOIN pg_catalog.pg_class c
ON stxrelid = c.oid
ORDER BY 1, 2, 3;

Thanks,
Tatsuro Yamada

Re: SyncRepLock acquired exclusively in default configuration

[Fujii Masao]

On 2020/08/27 15:59, Asim Praveen wrote:



On 26-Aug-2020, at 11:10 PM, Fujii Masao  wrote:

I added the following comments based on the suggestion by Sawada-san upthread. 
Thought?

+* Since this routine gets called every commit time, it's important to
+* exit quickly if sync replication is not requested. So we check
+* WalSndCtl->sync_standbys_define without the lock and exit
+* immediately if it's false. If it's true, we check it again later
+* while holding the lock, to avoid the race condition described
+* in SyncRepUpdateSyncStandbysDefined().



+1.  May I suggest the following addition to the above comment (feel free to
rephrase / reject)?

"If sync_standbys_defined was being set from false to true and we observe it as
false, it ok to skip the wait.  Replication was async and it is in the process
of being changed to sync, due to user request.  Subsequent commits will observe
the change and start waiting.”


Thanks for the suggestion! I'm not sure if it's worth adding this because
it seems obvious thing. But maybe you imply that we need to comment
why the lock is not necessary when sync_standbys_defined is false. Right?
If so, what about updating the comments as follows?

+* Since this routine gets called every commit time, it's important to
+* exit quickly if sync replication is not requested. So we check
+* WalSndCtl->sync_standbys_defined flag without the lock and exit
+* immediately if it's false. If it's true, we need to check it again 
later
+* while holding the lock, to check the flag and operate the sync rep
+* queue atomically. This is necessary to avoid the race condition
+* described in SyncRepUpdateSyncStandbysDefined(). On the other
+* hand, if it's false, the lock is not necessary because we don't touch
+* the queue.





Attached is the updated version of the patch. I didn't change how to
fix the issue. But I changed the check for fast exit so that it's called
before setting the "mode", to avoid a few cycle.




Looks good to me.  There is a typo in the comment:

 s/sync_standbys_define/sync_standbys_defined/


Fixed. Thanks!

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
diff --git a/src/backend/replication/syncrep.c 
b/src/backend/replication/syncrep.c
index df1e341c76..6e8c76537a 100644
--- a/src/backend/replication/syncrep.c
+++ b/src/backend/replication/syncrep.c
@@ -158,18 +158,30 @@ SyncRepWaitForLSN(XLogRecPtr lsn, bool commit)
 */
Assert(InterruptHoldoffCount > 0);
 
+   /*
+* Fast exit if user has not requested sync replication, or there are no
+* sync replication standby names defined.
+*
+* Since this routine gets called every commit time, it's important to
+* exit quickly if sync replication is not requested. So we check
+* WalSndCtl->sync_standbys_defined flag without the lock and exit
+* immediately if it's false. If it's true, we need to check it again 
later
+* while holding the lock, to check the flag and operate the sync rep
+* queue atomically. This is necessary to avoid the race condition
+* described in SyncRepUpdateSyncStandbysDefined(). On the other
+* hand, if it's false, the lock is not necessary because we don't touch
+* the queue.
+*/
+   if (!SyncRepRequested() ||
+   !((volatile WalSndCtlData *) WalSndCtl)->sync_standbys_defined)
+   return;
+
/* Cap the level for anything other than commit to remote flush only. */
if (commit)
mode = SyncRepWaitMode;
else
mode = Min(SyncRepWaitMode, SYNC_REP_WAIT_FLUSH);
 
-   /*
-* Fast exit if user has not requested sync replication.
-*/
-   if (!SyncRepRequested())
-   return;
-
Assert(SHMQueueIsDetached(&(MyProc->syncRepLinks)));
Assert(WalSndCtl != NULL);
 

Re: New default role- 'pg_read_all_data'

[Steven Pousty]

On Thu, Aug 27, 2020 at 5:30 PM Stephen Frost  wrote:

> Greetings,
>
> There's no shortage of requests and responses regarding how to have a
> 'read all of the data' role in PG, with various hacks involving "GRANT
> ALL" and "ALTER DEFAULT PRIVILEGES" to "solve" this, neither of which
> really works long term ("GRANT ALL" is one-time, and "ALTER DEFAULT"
> only helps for the roles that exist today).
>
> Now that we have the default role system, we can provide a proper
> solution to this oft-requested capability.
>
> This patch adds a default role to meet specifically that use-case, in
> the long-term, by explicitly allowing SELECT rights on all relations,
> and USAGE rights on all schemas, for roles who are members of the new
> 'pg_read_all_data' role.
>
> No effort is made to prevent a user who has this role from writing data-
> that's up to the admin, but this will allow someone to use pg_dump or
> pg_dumpall in a much more reliable manner to make sure that the entire
> database is able to be exported for the purpose of backups, upgrades, or
> other common use-cases, without having to have that same user be a PG
> superuser.
>
> This role is given the Bypass RLS right, though to use it effectively, a
> user would need to pass '--role=pg_read_all_data' to pg_dump/pg_dumpall,
> since role attributes are not checked as part of role membership.
>
>
This will be much appreciated from an app developers perspective. It makes
life so much easier to "do the right thing" in terms of giving read only
webapps the right permissions.

New default role- 'pg_read_all_data'

[Stephen Frost]

Greetings,

There's no shortage of requests and responses regarding how to have a
'read all of the data' role in PG, with various hacks involving "GRANT
ALL" and "ALTER DEFAULT PRIVILEGES" to "solve" this, neither of which
really works long term ("GRANT ALL" is one-time, and "ALTER DEFAULT"
only helps for the roles that exist today).

Now that we have the default role system, we can provide a proper
solution to this oft-requested capability.

This patch adds a default role to meet specifically that use-case, in
the long-term, by explicitly allowing SELECT rights on all relations,
and USAGE rights on all schemas, for roles who are members of the new
'pg_read_all_data' role.

No effort is made to prevent a user who has this role from writing data-
that's up to the admin, but this will allow someone to use pg_dump or
pg_dumpall in a much more reliable manner to make sure that the entire
database is able to be exported for the purpose of backups, upgrades, or
other common use-cases, without having to have that same user be a PG
superuser.

This role is given the Bypass RLS right, though to use it effectively, a
user would need to pass '--role=pg_read_all_data' to pg_dump/pg_dumpall,
since role attributes are not checked as part of role membership.

Thoughts?

Will add to the September CF.

Thanks,

Stephen
diff --git a/doc/src/sgml/user-manag.sgml b/doc/src/sgml/user-manag.sgml
index 829decd883..09b3cd6cb8 100644
--- a/doc/src/sgml/user-manag.sgml
+++ b/doc/src/sgml/user-manag.sgml
@@ -517,6 +517,12 @@ DROP ROLE doomed_role;
   
  
  
+  
+   pg_read_all_data
+   Read all data (tables, sequences), even without having explicit
+   GRANT rights to the object.  Implicitly includes USAGE rights on all
+   schemas.
+  
   
pg_read_all_settings
Read all configuration variables, even those normally visible only to
diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index c626161408..aca1deeca8 100644
--- a/src/backend/catalog/aclchk.c
+++ b/src/backend/catalog/aclchk.c
@@ -3849,6 +3849,15 @@ pg_class_aclmask(Oid table_oid, Oid roleid,
 
 	ReleaseSysCache(tuple);
 
+	/*
+	 * Check if ACL_SELECT is being checked and, if so, and not set already
+	 * as part of the result, then check if the user is a member of the
+	 * pg_read_all_data role, which allows read access to all relations.
+	 */
+	if (mask & ACL_SELECT && !(result & ACL_SELECT) &&
+		has_privs_of_role(roleid, DEFAULT_ROLE_READ_ALL_DATA))
+		result |= ACL_SELECT;
+
 	return result;
 }
 
@@ -4175,6 +4184,14 @@ pg_namespace_aclmask(Oid nsp_oid, Oid roleid,
 
 	ReleaseSysCache(tuple);
 
+	/*
+	 * Check if ACL_USAGE is being checked and, if so, and not set already
+	 * as part of the result, then check if the user is a member of the
+	 * pg_read_all_data role, which allows usage access to all schemas.
+	 */
+	if (mask & ACL_USAGE && !(result & ACL_USAGE) &&
+		has_privs_of_role(roleid, DEFAULT_ROLE_READ_ALL_DATA))
+		result |= ACL_USAGE;
 	return result;
 }
 
diff --git a/src/include/catalog/pg_authid.dat b/src/include/catalog/pg_authid.dat
index 7c08851550..0dc4b2baec 100644
--- a/src/include/catalog/pg_authid.dat
+++ b/src/include/catalog/pg_authid.dat
@@ -25,6 +25,11 @@
   rolcreaterole => 'f', rolcreatedb => 'f', rolcanlogin => 'f',
   rolreplication => 'f', rolbypassrls => 'f', rolconnlimit => '-1',
   rolpassword => '_null_', rolvaliduntil => '_null_' },
+{ oid => '3435', oid_symbol => 'DEFAULT_ROLE_READ_ALL_DATA',
+  rolname => 'pg_read_all_data', rolsuper => 'f', rolinherit => 't',
+  rolcreaterole => 'f', rolcreatedb => 'f', rolcanlogin => 'f',
+  rolreplication => 'f', rolbypassrls => 't', rolconnlimit => '-1',
+  rolpassword => '_null_', rolvaliduntil => '_null_' },
 { oid => '3374', oid_symbol => 'DEFAULT_ROLE_READ_ALL_SETTINGS',
   rolname => 'pg_read_all_settings', rolsuper => 'f', rolinherit => 't',
   rolcreaterole => 'f', rolcreatedb => 'f', rolcanlogin => 'f',
diff --git a/src/test/regress/expected/privileges.out b/src/test/regress/expected/privileges.out
index 3ec22c20ea..411525bcd1 100644
--- a/src/test/regress/expected/privileges.out
+++ b/src/test/regress/expected/privileges.out
@@ -24,8 +24,10 @@ CREATE USER regress_priv_user2;
 CREATE USER regress_priv_user3;
 CREATE USER regress_priv_user4;
 CREATE USER regress_priv_user5;
+CREATE USER regress_priv_user6;
 CREATE USER regress_priv_user5;	-- duplicate
 ERROR:  role "regress_priv_user5" already exists
+GRANT pg_read_all_data TO regress_priv_user6;
 CREATE GROUP regress_priv_group1;
 CREATE GROUP regress_priv_group2 WITH USER regress_priv_user1, regress_priv_user2;
 ALTER GROUP regress_priv_group1 ADD USER regress_priv_user4;
@@ -129,6 +131,21 @@ SELECT * FROM atest2 WHERE ( col1 IN ( SELECT b FROM atest1 ) );
 --+--
 (0 rows)
 
+SET SESSION AUTHORIZATION regress_priv_user6;
+SELECT * FROM atest1; -- ok
+ a |  b  
+---+-
+ 1 | two
+ 1 | two
+(2 rows)
+
+SELECT * FROM atest2; -- ok
+ col1 | col2 
+--+--

Disk-based hash aggregate's cost model

[Peter Geoghegan]

We have a Postgres 13 open item for Disk-based hash aggregate, which
is the only non-trivial open item. There is a general concern about
how often we get disk-based hash aggregation when work_mem is
particularly low, and recursion seems unavoidable. This is generally
thought to be a problem in the costing.

Tomas Vondra did some testing of the patch which led to discussion of
this on the hash agg GUC megathread, here:

https://www.postgresql.org/message-id/20200724012248.y77rpqc73agrsvb3@development

I am starting this new thread to deal with the open item.

Any progress on this, Jeff? Please correct or expand on my summary of
the open item if I got something wrong.

Thanks
-- 
Peter Geoghegan

[PATCH] Explicit null dereferenced (src/backend/access/heap/heaptoast.c)

[Ranier Vilela]

Hi,

Per Coverity.

When "Prepare for toasting", it is necessary to turn off the flag
TOAST_NEEDS_DELETE_OLD,
if there is no need to delete external values from the old tuple, otherwise,
there are dereference NULL at toast_helper.c (on toast_tuple_cleanup
function).


regards,
Ranier Vilela


fix_null_dereference_heaptoast.patch
Description: Binary data

Re: XMAX_LOCK_ONLY and XMAX_COMMITTED (fk/multixact code)

[Peter Geoghegan]

On Thu, Aug 27, 2020 at 5:06 PM Mark Dilger
 wrote:
> These two are both checked in verify_heapam.c.  The point is that the system 
> will also refuse to write out pages that have this corruption.  The Asserts 
> could be converted to panics or whatever, but that has other more serious 
> consequences.  Did you have something else in mind?

Oh, I see -- you propose to add both an assert to hio.c, as well as a
check to amcheck itself. That seems like the right thing to do.

Thanks

-- 
Peter Geoghegan

Re: XMAX_LOCK_ONLY and XMAX_COMMITTED (fk/multixact code)

[Mark Dilger]

> On Aug 27, 2020, at 4:58 PM, Peter Geoghegan  wrote:
> 
> On Thu, Aug 27, 2020 at 4:57 PM Mark Dilger
>  wrote:
>> The amcheck patch has Asserts in hio.c that purport to disallow writing 
>> invalid header bits to disk.
> 
> Can it also be a runtime check for the verification process? I think
> that we can easily afford to be fairly exhaustive about stuff like
> this.

These two are both checked in verify_heapam.c.  The point is that the system 
will also refuse to write out pages that have this corruption.  The Asserts 
could be converted to panics or whatever, but that has other more serious 
consequences.  Did you have something else in mind?

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: Clang Address Sanitizer (Postgres14) Detected Memory Leaks

[Tom Lane]

Ranier Vilela  writes:
> More reports.
> Memory Sanitizer:
> running bootstrap script ... ==40179==WARNING: MemorySanitizer:
> use-of-uninitialized-value

If you're going to run tests like that, you need to account for the
known exceptions shown in src/tools/valgrind.supp.

regards, tom lane

Re: XMAX_LOCK_ONLY and XMAX_COMMITTED (fk/multixact code)

[Peter Geoghegan]

On Thu, Aug 27, 2020 at 4:57 PM Mark Dilger
 wrote:
> The amcheck patch has Asserts in hio.c that purport to disallow writing 
> invalid header bits to disk.

Can it also be a runtime check for the verification process? I think
that we can easily afford to be fairly exhaustive about stuff like
this.

-- 
Peter Geoghegan

Re: XMAX_LOCK_ONLY and XMAX_COMMITTED (fk/multixact code)

[Mark Dilger]

> On Aug 27, 2020, at 4:47 PM, Peter Geoghegan  wrote:
> 
> On Wed, Aug 26, 2020 at 12:16 PM Alvaro Herrera
>  wrote:
>> We could do this in stable branches, if there were any reports that
>> this inconsistency is happening in real world databases.
> 
> I hope that the new heapam amcheck stuff eventually leads to our
> having total (or near total) certainty about what correct on-disk
> states are possible, regardless of the exact pg_upgrade + minor
> version paths. We should take a strict line on this stuff where
> possible. If that turns out to be wrong in some detail, then it's
> relatively easy to fix as a bug in amcheck itself.
> 
> There is a high cost to allowing ambiguity about what heapam states
> are truly legal/possible. It makes future development projects harder.

The amcheck patch has Asserts in hio.c that purport to disallow writing invalid 
header bits to disk.  The combination being discussed here is not disallowed, 
but if there is consensus that it is an illegal combination, we could certainly 
add it:

diff --git a/src/backend/access/heap/hio.c b/src/backend/access/heap/hio.c
index aa3f14c019..ca357410a2 100644
--- a/src/backend/access/heap/hio.c
+++ b/src/backend/access/heap/hio.c
@@ -47,6 +47,17 @@ RelationPutHeapTuple(Relation relation,
 */
Assert(!token || HeapTupleHeaderIsSpeculative(tuple->t_data));
 
+   /*
+* Do not allow tuples with invalid combinations of hint bits to be 
placed
+* on a page.  These combinations are detected as corruption by the
+* contrib/amcheck logic, so if you disable one or both of these
+* assertions, make corresponding changes there.
+*/
+   Assert(!((tuple->t_data->t_infomask & HEAP_XMAX_LOCK_ONLY) &&
+(tuple->t_data->t_infomask2 & HEAP_KEYS_UPDATED)));
+   Assert(!((tuple->t_data->t_infomask & HEAP_XMAX_COMMITTED) &&
+(tuple->t_data->t_infomask & HEAP_XMAX_IS_MULTI)));
+
/* Add the tuple to the page */
pageHeader = BufferGetPage(buffer);
 

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: list of extended statistics on psql

[Alvaro Herrera]

+1 for the general idea, and +1 for \dX being the syntax to use

IMO the per-type columns should show both the type being enabled as
well as it being built.

(How many more stat types do we expect -- Tomas?  I wonder if having one
column per type is going to scale in the long run.)

Also, the stat obj name column should be first, followed by a single
column listing both table and columns that it applies to.  Keep in mind
that in the future we might want to add stats that cross multiple tables
-- that's why the CREATE syntax is the way it is.  So we should give
room for that in psql's display too.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Clang Address Sanitizer (Postgres14) Detected Memory Leaks

[Ranier Vilela]

More reports.
Memory Sanitizer:

running bootstrap script ... ==40179==WARNING: MemorySanitizer:
use-of-uninitialized-value
#0 0x538cfc1 in pg_comp_crc32c_sb8
/usr/src/postgres/src/port/pg_crc32c_sb8.c:80:4
#1 0x533a0c0 in pg_comp_crc32c_choose
/usr/src/postgres/src/port/pg_crc32c_sse42_choose.c:61:9
#2 0xebbdae in BootStrapXLOG
/usr/src/postgres/src/backend/access/transam/xlog.c:5293:2
#3 0xfc5867 in AuxiliaryProcessMain
/usr/src/postgres/src/backend/bootstrap/bootstrap.c:437:4
#4 0x26a12c3 in main /usr/src/postgres/src/backend/main/main.c:201:3
#5 0x7f035d0e90b2 in __libc_start_main
/build/glibc-YYA7BZ/glibc-2.31/csu/../csu/libc-start.c:308:16
#6 0x495afd in _start
(/usr/src/postgres/tmp_install/usr/local/pgsql/bin/postgres+0x495afd)

  Uninitialized value was stored to memory at
#0 0x538cbaa in pg_comp_crc32c_sb8
/usr/src/postgres/src/port/pg_crc32c_sb8.c:72:15
#1 0x533a0c0 in pg_comp_crc32c_choose
/usr/src/postgres/src/port/pg_crc32c_sse42_choose.c:61:9
#2 0xebbdae in BootStrapXLOG
/usr/src/postgres/src/backend/access/transam/xlog.c:5293:2
#3 0xfc5867 in AuxiliaryProcessMain
/usr/src/postgres/src/backend/bootstrap/bootstrap.c:437:4
#4 0x26a12c3 in main /usr/src/postgres/src/backend/main/main.c:201:3
#5 0x7f035d0e90b2 in __libc_start_main
/build/glibc-YYA7BZ/glibc-2.31/csu/../csu/libc-start.c:308:16

  Uninitialized value was stored to memory at
#0 0x538c836 in pg_comp_crc32c_sb8
/usr/src/postgres/src/port/pg_crc32c_sb8.c:57:11
#1 0x533a0c0 in pg_comp_crc32c_choose
/usr/src/postgres/src/port/pg_crc32c_sse42_choose.c:61:9
#2 0xebbdae in BootStrapXLOG
/usr/src/postgres/src/backend/access/transam/xlog.c:5293:2
#3 0xfc5867 in AuxiliaryProcessMain
/usr/src/postgres/src/backend/bootstrap/bootstrap.c:437:4
#4 0x26a12c3 in main /usr/src/postgres/src/backend/main/main.c:201:3
#5 0x7f035d0e90b2 in __libc_start_main
/build/glibc-YYA7BZ/glibc-2.31/csu/../csu/libc-start.c:308:16

  Uninitialized value was stored to memory at
#0 0x49b666 in __msan_memcpy
(/usr/src/postgres/tmp_install/usr/local/pgsql/bin/postgres+0x49b666)
#1 0xebbb70 in BootStrapXLOG
/usr/src/postgres/src/backend/access/transam/xlog.c:5288:2
#2 0xfc5867 in AuxiliaryProcessMain
/usr/src/postgres/src/backend/bootstrap/bootstrap.c:437:4
#3 0x26a12c3 in main /usr/src/postgres/src/backend/main/main.c:201:3
#4 0x7f035d0e90b2 in __libc_start_main
/build/glibc-YYA7BZ/glibc-2.31/csu/../csu/libc-start.c:308:16

  Uninitialized value was created by an allocation of 'checkPoint' in the
stack frame of function 'BootStrapXLOG'
#0 0xeb9f50 in BootStrapXLOG
/usr/src/postgres/src/backend/access/transam/xlog.c:5194

This line solve the alert:
(xlog.c) 5193:
memset(&checkPoint, 0, sizeof(checkPoint));

I'm starting to doubt this tool.

regards,
Ranier Vilela

Re: XMAX_LOCK_ONLY and XMAX_COMMITTED (fk/multixact code)

[Peter Geoghegan]

On Wed, Aug 26, 2020 at 12:16 PM Alvaro Herrera
 wrote:
> We could do this in stable branches, if there were any reports that
> this inconsistency is happening in real world databases.

I hope that the new heapam amcheck stuff eventually leads to our
having total (or near total) certainty about what correct on-disk
states are possible, regardless of the exact pg_upgrade + minor
version paths. We should take a strict line on this stuff where
possible. If that turns out to be wrong in some detail, then it's
relatively easy to fix as a bug in amcheck itself.

There is a high cost to allowing ambiguity about what heapam states
are truly legal/possible. It makes future development projects harder.

-- 
Peter Geoghegan

Re: list of extended statistics on psql

[Tatsuro Yamada]

Hi Julien!
 


Thanks also for the documentation and regression tests.  This overall looks
good, I just have a two comments:



Thank you for reviewing the patch! :-D



- there's a whitespace issue in the documentation part:

add_list_extended_stats_for_psql_by_dX_command.patch:10: tab in indent.
  
warning: 1 line adds whitespace errors.



Oops, I forgot to use "git diff --check". I fixed it.

 

- You're sorting the output on schema, table, extended statistics and columns
   but I think the last one isn't required since extended statistics names are
   unique.



You are right.
The sort key "columns" was not necessary so I removed it.

Attached new patch includes the above two fixes:

  - Fix whitespace issue in the documentation part
  - Remove unnecessary sort key from the query
 (ORDER BY 1, 2, 3, 4 -> ORDER BY 1, 2, 3)


Thanks,
Tatsuro Yamada

diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index fc16e6c..8b0568c 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -1893,6 +1893,18 @@ testdb=>
 
 
   
+  
+  
+\dX [ pattern ]
+
+
+Lists extended statistics.
+If pattern
+is specified, only those extended statistics whose names match the 
pattern
+are listed.
+
+
+  
 
   
 \dy[+] [ pattern ]
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 9902a4a..dcc9fba 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -929,6 +929,9 @@ exec_command_d(PsqlScanState scan_state, bool 
active_branch, const char *cmd)
else
success = listExtensions(pattern);
break;
+   case 'X':   /* Extended Statistics 
*/
+   success = listExtendedStats(pattern);
+   break;
case 'y':   /* Event Triggers */
success = listEventTriggers(pattern, 
show_verbose);
break;
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index d81f157..e43241f 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -4368,6 +4368,74 @@ listEventTriggers(const char *pattern, bool verbose)
 }
 
 /*
+ * \dX
+ *
+ * Briefly describes extended statistics.
+ */
+bool
+listExtendedStats(const char *pattern)
+{
+   PQExpBufferData buf;
+   PGresult   *res;
+   printQueryOpt myopt = pset.popt;
+
+   if (pset.sversion < 10)
+   {
+   charsverbuf[32];
+
+   pg_log_error("The server (version %s) does not support extended 
statistics.",
+formatPGVersionNumber(pset.sversion, 
false,
+   
   sverbuf, sizeof(sverbuf)));
+   return true;
+   }
+
+   initPQExpBuffer(&buf);
+   printfPQExpBuffer(&buf,
+ "SELECT \n"
+ 
"stxnamespace::pg_catalog.regnamespace AS \"%s\", \n"
+ "c.relname AS \"%s\", \n"
+ "stxname AS \"%s\", \n"
+ "(SELECT 
pg_catalog.string_agg(pg_catalog.quote_ident(attname),', ') \n"
+ " FROM pg_catalog.unnest(stxkeys) 
s(attnum) \n"
+ " JOIN pg_catalog.pg_attribute a ON 
(stxrelid = a.attrelid AND \n"
+ " a.attnum = s.attnum AND NOT 
attisdropped)) AS \"%s\", \n"
+ "'d' = any(stxkind) AS \"%s\", \n"
+ "'f' = any(stxkind) AS \"%s\", \n"
+ "'m' = any(stxkind) AS \"%s\"  \n"
+ "FROM pg_catalog.pg_statistic_ext \n"
+ "INNER JOIN pg_catalog.pg_class c \n"
+ "ON stxrelid = c.oid \n",
+ gettext_noop("Schema"),
+ gettext_noop("Table"),
+ gettext_noop("Name"),
+ gettext_noop("Columns"),
+ gettext_noop("Ndistinct"),
+ gettext_noop("Dependencies"),
+ gettext_noop("MCV"));
+
+   processSQLNamePattern(pset.db, &buf, pattern, false,
+ false, NULL,
+ "stxname", NULL,
+

Re: Autovac cancellation is broken in v14

[Andres Freund]

Hi,

On 2020-08-27 16:20:30 -0400, Jeff Janes wrote:
> On Thu, Aug 27, 2020 at 3:10 PM Jeff Janes  wrote:
> 
> > If I create a large table with "CREATE TABLE ... AS SELECT ... from
> > generate_series(1,3e7)" with no explicit transactions, then once it is done
> > I wait for autovac to kick in, then when I try to build an index on that
> > table (or drop the table) the autovac doesn't go away on its own.
> >
> 
> After a bit more poking at this, I think we are checking if we ourselves
> are an autovac process, not doing the intended check of whether the other
> guy is one.

Ugh, good catch.


> Where would be a good spot to add a regression test for this?
> "isolation_regression" ?

I'm not immediately sure how we could write a good test for this,
particularly not in the isolation tests. We'd basically have to make
sure that a table needs autovacuuming, then sleep for long enough for
autovacuum to have come around, and block autovacuum from making
progress. That latter is doable by holding a pin on a page it needs to
freeze, e.g. using a cursor.  I suspect all of that would at least
require a TAP test, and might still be too fragile.

Other ideas?

Regards,

Andres

RE: [EXTERNAL] Re: WIP: WAL prefetch (another approach)

[Sait Talha Nisanci]

Hi Stephen,

OS version is Ubuntu 18.04.5 LTS.
Filesystem is ext4 and block size is 4KB.

Talha.

-Original Message-
From: Stephen Frost  
Sent: Thursday, August 27, 2020 4:56 PM
To: Sait Talha Nisanci 
Cc: Thomas Munro ; Tomas Vondra 
; Dmitry Dolgov <9erthali...@gmail.com>; David 
Steele ; Andres Freund ; Alvaro 
Herrera ; pgsql-hackers 
Subject: Re: [EXTERNAL] Re: WIP: WAL prefetch (another approach)

Greetings,

* Sait Talha Nisanci (sait.nisa...@microsoft.com) wrote:
> I have run some benchmarks for this patch. Overall it seems that there is a 
> good improvement with the patch on recovery times:

Maybe I missed it somewhere, but what's the OS/filesystem being used here..?  
What's the filesystem block size..?

Thanks,

Stephen

Re: recovering from "found xmin ... from before relfrozenxid ..."

[Mark Dilger]

> On Aug 26, 2020, at 4:36 AM, Ashutosh Sharma  wrote:
> 
> This patch also takes care of all the other review comments from - [1].
> 
> [1] - 
> https://www.postgresql.org/message-id/CA%2Bfd4k6%2BJWq2MfQt5b7fSJ2wMvCes9TRfbDhVO_fQP9B8JJRAA%40mail.gmail.com
> 
> 
> --
> With Regards,
> Ashutosh Sharma
> EnterpriseDB:http://www.enterprisedb.com
> 


Hi Ashutosh,

I took a look at the v8 patch, created a commitfest entry [1] because I did not 
find one already existent, and have the following review comments:


HeapTupleForceOption should be added to src/tools/pgindent/typedefs.list.


The tidcmp function can be removed, and ItemPointerCompare used directly by 
qsort as:

-   qsort((void*) tids, ntids, sizeof(ItemPointerData), tidcmp);
+   qsort((void*) tids, ntids, sizeof(ItemPointerData),
+ (int (*) (const void *, const void *)) 
ItemPointerCompare);


sanity_check_tid_array() has two error messages:

  "array must not contain nulls"
  "empty tid array"

I would change the first to say "tid array must not contain nulls", as "tid" is 
the name of the parameter being checked.  It is also more consistent with the 
second error message, but that doesn't matter to me so much, as I'd argue for 
removing the second check.  I don't see why an empty array should draw an 
error.  It seems more reasonable to just return early since there is no work to 
do.  Consider if somebody uses a function that returns the tids for all corrupt 
tuples in a table, aggregates that into an array, and hands that to this 
function.  It doesn't seem like an error for that aggregated array to have zero 
elements in it.  I suppose you could emit a NOTICE in this case?


Upthread:
> On Aug 13, 2020, at 12:03 PM, Robert Haas  wrote:
> 
>> This looks like a very good suggestion to me. I will do this change in
>> the next version. Just wondering if we should be doing similar changes
>> in other contrib modules (like pgrowlocks, pageinspect and
>> pgstattuple) as well?
> 
> It seems like it should be consistent, but I'm not sure the proposed
> change is really an improvement.

You have used Asim's proposed check:

if (rel->rd_amhandler != HEAP_TABLE_AM_HANDLER_OID)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
 errmsg("only the relation using heap_tableam_handler is 
supported")));

which Robert seems unenthusiastic about, but if you are going that direction, I 
think at least the language of the error message should be changed. I recommend 
something like:

if (rel->rd_amhandler != HEAP_TABLE_AM_HANDLER_OID)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-errmsg("only the relation using 
heap_tableam_handler is supported")));
+errmsg("\"%s\" does not use a heap access 
method",
+   RelationGetRelationName(rel;

where "a heap access method" could also be written as "a heap table type access 
method", "a heap table compatible access method", and so forth.  There doesn't 
seem to be enough precedent to dictate exactly how to phrase this, or perhaps 
I'm just not looking in the right place.


The header comment for function find_tids_one_page should state the requirement 
that the tids array must be sorted.


The heap_force_common function contains multiple ereport(NOTICE,...) within a 
critical section.  I don't think that is normal practice.  Can those reports be 
buffered until after the critical section is exited?  You also have a 
CHECK_FOR_INTERRUPTS within the critical section.

[1] https://commitfest.postgresql.org/29/2700/
—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: [EXTERNAL] Re: WIP: WAL prefetch (another approach)

[Stephen Frost]

Greetings,

* Robert Haas (robertmh...@gmail.com) wrote:
> On Thu, Aug 27, 2020 at 2:51 PM Stephen Frost  wrote:
> > > Hm? At least earlier versions didn't do prefetching for records with an 
> > > fpw, and only for subsequent records affecting the same or if not in s_b 
> > > anymore.
> >
> > We don't actually read the page when we're replaying an FPW though..?
> > If we don't read it, and we entirely write the page from the FPW, how is
> > pre-fetching helping..?
> 
> Suppose there is a checkpoint. Then we replay a record with an FPW,
> pre-fetching nothing. Then the buffer gets evicted from
> shared_buffers, and maybe the OS cache too. Then, before the next
> checkpoint, we again replay a record for the same page. At this point,
> pre-fetching should be helpful.

Sure- but if we're talking about 25GB of WAL, on a server that's got
32GB, then why would those pages end up getting evicted from memory
entirely?  Particularly, enough of them to end up with such a huge
difference in replay time..

I do agree that if we've got more outstanding WAL between checkpoints
than the system's got memory then that certainly changes things, but
that wasn't what I understood the case to be here.

> Admittedly, I don't quite understand whether that is what is happening
> in this test case, or why SDD vs. HDD should make any difference. But
> there doesn't seem to be any reason why it doesn't make sense in
> theory.

I agree that this could be a reason, but it doesn't seem to quite fit in
this particular case given the amount of memory and WAL.  I'm suspecting
that it's something else and I'd very much like to know if it's a
general "this applies to all (most?  a lot of?) SSDs because the
hardware has a larger than 8KB page size and therefore the kernel has to
read it", or if it's something odd about this particular system and
doesn't apply generally.

Thanks,

Stephen


signature.asc
Description: PGP signature

Re: Autovac cancellation is broken in v14

[Jeff Janes]

On Thu, Aug 27, 2020 at 3:10 PM Jeff Janes  wrote:

> If I create a large table with "CREATE TABLE ... AS SELECT ... from
> generate_series(1,3e7)" with no explicit transactions, then once it is done
> I wait for autovac to kick in, then when I try to build an index on that
> table (or drop the table) the autovac doesn't go away on its own.
>

After a bit more poking at this, I think we are checking if we ourselves
are an autovac process, not doing the intended check of whether the other
guy is one.

Where would be a good spot to add a regression test for this?
"isolation_regression" ?
Cheers,

Jeff


fix_autovac_cancel.patch
Description: Binary data

Re: [EXTERNAL] Re: WIP: WAL prefetch (another approach)

[Robert Haas]

On Thu, Aug 27, 2020 at 2:51 PM Stephen Frost  wrote:
> > Hm? At least earlier versions didn't do prefetching for records with an 
> > fpw, and only for subsequent records affecting the same or if not in s_b 
> > anymore.
>
> We don't actually read the page when we're replaying an FPW though..?
> If we don't read it, and we entirely write the page from the FPW, how is
> pre-fetching helping..?

Suppose there is a checkpoint. Then we replay a record with an FPW,
pre-fetching nothing. Then the buffer gets evicted from
shared_buffers, and maybe the OS cache too. Then, before the next
checkpoint, we again replay a record for the same page. At this point,
pre-fetching should be helpful.

Admittedly, I don't quite understand whether that is what is happening
in this test case, or why SDD vs. HDD should make any difference. But
there doesn't seem to be any reason why it doesn't make sense in
theory.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: recovering from "found xmin ... from before relfrozenxid ..."

[Robert Haas]

On Wed, Aug 26, 2020 at 10:26 PM Ashutosh Sharma  wrote:
> Okay, point noted.

I spent some time today working on this patch. I'm fairly happy with
it now and intend to commit it if nobody sees a big problem with that.
Per discussion, I do not intend to back-patch at this time. The two
most significant changes I made to your version are:

1. I changed things around to avoid using any form of ereport() in a
critical section. I'm not actually sure whether it is project policy
to avoid ereport(NOTICE, ...) or similar in a critical section, but it
seems prudent, because if anything fails in a critical section, we
will PANIC, so doing fewer things there seems prudent.

2. I changed the code so that it does not try to follow redirected
line pointers; instead, it skips them with an appropriate message, as
we were already doing for dead and unused line pointers. I think the
way you had it coded might've been my suggestion originally, but the
more I looked into it the less I liked it. One problem is that it
didn't match the docs. A second is that following a corrupted line
pointer might index off the end of the line pointer array, and while
that probably shouldn't happen, we are talking about corruption
recovery here. Then I realized that, as you coded it, if the line
pointer was redirected to a line pointer that is in turn dead (or
unused, if there's corruption) the user would get a NOTICE complaining
about a TID they hadn't specified, which seems like it would be very
confusing. I thought about trying to fix all that stuff, but it just
didn't seem worth it, because I can't think of a good reason to pass
this function the TID of a redirected line pointer in the first place.
If you're doing surgery, you should probably specify the exact thing
upon which you want to operate, not some other thing that points to
it.

Here is a list of other changes I made:

* Added a .gitignore file.
* Renamed the regression test file from pg_surgery to heap_surgery to
match the name of the single C source file we currently have.
* Capitalized TID in a few places.
* Ran pgindent.
* Adjusted various comments.
* Removed the check for an empty TID array. I don't see any reason why
this should be an error case and I don't see much precedent for having
such a check.
* Fixed the code to work properly with that change and added a test case.
* Added a check that the array is not multi-dimensional.
* Put the AM type check before the relkind check, following existing precedent.
* Adjusted the check to use the AM OID rather than the handler OID,
following existing precedent. Fixed the message wording accordingly.
* Changed the documentation wording to say less about specific
recovery procedures and focus more on the general idea that this is
dangerous.
* Removed all but one of the test cases that checked what happens if
you use this on a non-heap; three tests for basically the same thing
seemed excessive.
* Added some additional tests to improve code coverage. There are now
only a handful of lines not covered.
* Reorganized the test cases somewhat.

New patch attached.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


v9-0001-pg_surgery-rmh-based-on-ashutosh-sharma-v8.patch
Description: Binary data

Re: "cert" + clientcert=verify-ca in pg_hba.conf?

[Bruce Momjian]

On Thu, Aug 27, 2020 at 03:41:40PM -0400, Bruce Momjian wrote:
> On Thu, Aug 27, 2020 at 04:09:25PM +0900, Kyotaro Horiguchi wrote:
> > At Wed, 26 Aug 2020 18:36:50 -0400, Bruce Momjian  wrote 
> > in 
> > bruce> On Wed, Aug 26, 2020 at 06:13:23PM +0900, Kyotaro Horiguchi wrote:
> > > > At Tue, 25 Aug 2020 22:52:44 -0400, Bruce Momjian  
> > > > wrote in 
> > > > > > Because we think we need any named value for every alternatives
> > > > > > including the default value?
> > > > > 
> > > > > Well, not putting clientcert at all gives the default behavior, so why
> > > > > have clientcert=no-verify?
> > > > 
> > > > clientcert=verify-ca or verify-full don't allow absence of client
> > > > certificate. We need an option to allow the absence.
> > > 
> > > Isn't the option not specifying clientcert?  Here are some valid
> > > pg_hba.conf lines:
> > 
> > Sorry for the ambiguity. Perhaps I understand that we talked at
> > different objects.  I was mentioning about the option value that is
> > stored *internally*, concretely the values for the struct member
> > port->hba->clientcert. You are talking about the descriptive option in
> > pg_hba.conf.

Yes, I realize we need an internal vaue for this, but it doesn't need to
be visible to the user.

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

  The usefulness of a cup is in its emptiness, Bruce Lee

Re: "cert" + clientcert=verify-ca in pg_hba.conf?

[Bruce Momjian]

On Thu, Aug 27, 2020 at 04:09:25PM +0900, Kyotaro Horiguchi wrote:
> At Wed, 26 Aug 2020 18:36:50 -0400, Bruce Momjian  wrote in 
> bruce> On Wed, Aug 26, 2020 at 06:13:23PM +0900, Kyotaro Horiguchi wrote:
> > > At Tue, 25 Aug 2020 22:52:44 -0400, Bruce Momjian  
> > > wrote in 
> > > > > Because we think we need any named value for every alternatives
> > > > > including the default value?
> > > > 
> > > > Well, not putting clientcert at all gives the default behavior, so why
> > > > have clientcert=no-verify?
> > > 
> > > clientcert=verify-ca or verify-full don't allow absence of client
> > > certificate. We need an option to allow the absence.
> > 
> > Isn't the option not specifying clientcert?  Here are some valid
> > pg_hba.conf lines:
> 
> Sorry for the ambiguity. Perhaps I understand that we talked at
> different objects.  I was mentioning about the option value that is
> stored *internally*, concretely the values for the struct member
> port->hba->clientcert. You are talking about the descriptive option in
> pg_hba.conf.
> 
> Does the following discussion make sense?
> 
> We need to use the default value zero (=clientCertOff) for
> port->hba->clientcert to tell server to omit checking against CA if
> cert is not given.  I suppose that the value clientCertOff is labeled
> as "no-verify" since someone who developed this thought that that
> choice needs to be explicitly describable in pg_hba.conf. And my
> discussion was following that decision.
> 
> I understand that the label "no-verify" is not essential to specify
> the behavior, so I don't object to removing "no-verify" label itself
> if no one oppose to remove it.
> 
> My point here is just "are we OK to remove it?"

Yes, in PG 14.  Security is confusing enough, so having a mis-named
option that doesn't do anything more than just not specifying clientcert
is not useful and should be removed.

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

  The usefulness of a cup is in its emptiness, Bruce Lee

Autovac cancellation is broken in v14

[Jeff Janes]

If I create a large table with "CREATE TABLE ... AS SELECT ... from
generate_series(1,3e7)" with no explicit transactions, then once it is done
I wait for autovac to kick in, then when I try to build an index on that
table (or drop the table) the autovac doesn't go away on its own.

Bisects down to:

commit 5788e258bb26495fab65ff3aa486268d1c50b123
Author: Andres Freund 
Date:   Wed Jul 15 15:35:07 2020 -0700

snapshot scalability: Move PGXACT->vacuumFlags to
ProcGlobal->vacuumFlags.

Which makes sense given the parts of the code this touches, although I
don't understand exactly what the problem is.  The problem persists in HEAD
(77c7267c37).

Cheers,

Jeff

Re: [EXTERNAL] Re: WIP: WAL prefetch (another approach)

[Stephen Frost]

Greetings,

* Andres Freund (and...@anarazel.de) wrote:
> On August 27, 2020 11:26:42 AM PDT, Stephen Frost  wrote:
> >Is WAL FPW compression enabled..?  I'm trying to figure out how, given
> >what's been shared here, that replaying 25GB of WAL is being helped out
> >by 2.5x thanks to prefetch in the SSD case.  That prefetch is hurting
> >in
> >the HDD case entirely makes sense to me- we're spending time reading
> >pages from the HDD, which is entirely pointless work given that we're
> >just going to write over those pages entirely with FPWs.
> 
> Hm? At least earlier versions didn't do prefetching for records with an fpw, 
> and only for subsequent records affecting the same or if not in s_b anymore.

We don't actually read the page when we're replaying an FPW though..?
If we don't read it, and we entirely write the page from the FPW, how is
pre-fetching helping..?  I understood how it could be helpful for
filesystems which have a larger block size than ours (eg: zfs w/ 16kb
block sizes where the kernel needs to get the whole 16kb block when we
only write 8kb to it), but that's apparently not the case here.

So- what is it that pre-fetching is doing to result in such an
improvement?  Is there something lower level where the SSD physical
block size is coming into play, which is typically larger..?  I wouldn't
have thought so, but perhaps that's the case..

Thanks,

Stephen


signature.asc
Description: PGP signature

Re: [EXTERNAL] Re: WIP: WAL prefetch (another approach)

[Andres Freund]

Hi, 

On August 27, 2020 11:26:42 AM PDT, Stephen Frost  wrote:
>Is WAL FPW compression enabled..?  I'm trying to figure out how, given
>what's been shared here, that replaying 25GB of WAL is being helped out
>by 2.5x thanks to prefetch in the SSD case.  That prefetch is hurting
>in
>the HDD case entirely makes sense to me- we're spending time reading
>pages from the HDD, which is entirely pointless work given that we're
>just going to write over those pages entirely with FPWs.

Hm? At least earlier versions didn't do prefetching for records with an fpw, 
and only for subsequent records affecting the same or if not in s_b anymore.

Andres
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [EXTERNAL] Re: WIP: WAL prefetch (another approach)

[Stephen Frost]

Greetings,

* Sait Talha Nisanci (sait.nisa...@microsoft.com) wrote:
> OS version is Ubuntu 18.04.5 LTS.
> Filesystem is ext4 and block size is 4KB.

[...]

* Sait Talha Nisanci (sait.nisa...@microsoft.com) wrote:
> I have run some benchmarks for this patch. Overall it seems that there is a 
> good improvement with the patch on recovery times:
> 
> The VMs I used have 32GB RAM, pgbench is initialized with a scale factor 
> 3000(so it doesn’t fit to memory, ~45GB).
> 
> In order to avoid checkpoints during benchmark, max_wal_size(200GB) and 
> checkpoint_timeout(200 mins) are set to a high value. 
> 
> The run is cancelled when there is a reasonable amount of WAL ( > 25GB). The 
> recovery times are measured from the REDO logs.
> 
> I have tried combination of SSD, HDD, full_page_writes = on/off and 
> max_io_concurrency = 10/50, the recovery times are as follows (in seconds):
> 
>  No prefetch  | Default prefetch 
> values  |  Default + max_io_concurrency = 50
> SSD, full_page_writes = on852 301 
> 197
> SSD, full_page_writes = off   16421359
> 1391
> HDD, full_page_writes = on60276345
> 6390
> HDD, full_page_writes = off   738 275 
> 192
> 
> Default prefetch values:
> - Max_recovery_prefetch_distance = 256KB
> - Max_io_concurrency = 10
> 
> It probably makes sense to compare each row separately as the size of WAL can 
> be different.

Is WAL FPW compression enabled..?  I'm trying to figure out how, given
what's been shared here, that replaying 25GB of WAL is being helped out
by 2.5x thanks to prefetch in the SSD case.  That prefetch is hurting in
the HDD case entirely makes sense to me- we're spending time reading
pages from the HDD, which is entirely pointless work given that we're
just going to write over those pages entirely with FPWs.

Further, if there's 32GB of RAM, and WAL compression isn't enabled and
the WAL is only 25GB, then it's very likely that every page touched by
the WAL ends up in memory (shared buffers or fs cache), and with FPWs we
shouldn't ever need to actually read from the storage to get those
pages, right?  So how is prefetch helping so much..?

I'm not sure that the 'full_page_writes = off' tests are very
interesting in this case, since you're going to get torn pages and
therefore corruption and hopefully no one is running with that
configuration with this OS/filesystem.

Thanks,

Stephen


signature.asc
Description: PGP signature

Re: Clang UndefinedBehaviorSanitize (Postgres14) Detected undefined-behavior

[Alvaro Herrera]

On 2020-Aug-27, Ranier Vilela wrote:

> If we are passing a null pointer in these places and it should not be done,
> it is a sign that perhaps these calls should not or should not be made, and
> they can be avoided.

Feel free to send a patch.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Clang UndefinedBehaviorSanitize (Postgres14) Detected undefined-behavior

[Ranier Vilela]

Em qui., 27 de ago. de 2020 às 13:57, Alvaro Herrera <
alvhe...@2ndquadrant.com> escreveu:

> On 2020-Aug-27, Ranier Vilela wrote:
>
> > indexcmds.c (1162):
> > memcpy(part_oids, partdesc->oids, sizeof(Oid) * nparts);
>
> Looks legit, and at least per commit 13bba02271dc we do fix such things,
> even if it's useless in practice.
>
> Given that no buildfarm member has ever complained, this exercise seems
> pretty pointless.
>
See at:
https://postgrespro.com/list/thread-id/1870065
"NULL passed as an argument to memcmp() in parse_func.c

"

regards,
Ranier Vilela

Re: Deprecating postfix and factorial operators in PostgreSQL 13

[Mark Dilger]

> On Aug 27, 2020, at 9:16 AM, Tom Lane  wrote:
> 
> Mark Dilger  writes:
>> The deprecation warnings included in this patch warn that postfix operator 
>> support, along with both postfix ! and prefix !! factorial operators, will 
>> be removed in PostgreSQL 14.
> 
> The operator docs should say "use factorial() instead", or words to
> that effect.
> 
>   regards, tom lane

Yes, that is better.  Attached.



v2-0001-Adding-deprecation-notices.patch
Description: Binary data


—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: Clang UndefinedBehaviorSanitize (Postgres14) Detected undefined-behavior

[Ranier Vilela]

Em qui., 27 de ago. de 2020 às 13:57, Alvaro Herrera <
alvhe...@2ndquadrant.com> escreveu:

> On 2020-Aug-27, Ranier Vilela wrote:
>
> > indexcmds.c (1162):
> > memcpy(part_oids, partdesc->oids, sizeof(Oid) * nparts);
>
> Looks legit, and at least per commit 13bba02271dc we do fix such things,
> even if it's useless in practice.
>
> Given that no buildfarm member has ever complained, this exercise seems
> pretty pointless.
>
Hi Álvaro,
If we are passing a null pointer in these places and it should not be done,
it is a sign that perhaps these calls should not or should not be made, and
they can be avoided.
This would eliminate undefined behavior and save some cycles?

regards,
Ranier Vilela

Re: Clang UndefinedBehaviorSanitize (Postgres14) Detected undefined-behavior

[Alvaro Herrera]

On 2020-Aug-27, Ranier Vilela wrote:

> indexcmds.c (1162):
> memcpy(part_oids, partdesc->oids, sizeof(Oid) * nparts);

Looks legit, and at least per commit 13bba02271dc we do fix such things,
even if it's useless in practice.

Given that no buildfarm member has ever complained, this exercise seems
pretty pointless.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Clang Address Sanitizer (Postgres14) Detected Memory Leaks

[Ranier Vilela]

Em qui., 27 de ago. de 2020 às 12:46, Tom Lane  escreveu:

> Ranier Vilela  writes:
> > Is this something to worry about, or is it another problem with the
> > analysis tool, that nobody cares about?
>
> As far as the first one goes, I'd bet on buggy analysis tool.
> The complained-of allocation is evidently for the "extra" state
> associated with the timezone GUC variable, and AFAICS guc.c is
> quite careful not to leak those.  It is true that the block will
> still be allocated at process exit, but that doesn't make it a leak.
>
> I did not trace the second one in any detail, but I don't believe
> guc.c leaks sourcefile strings either.  There's only one place
> where it overwrites them, and that place frees the old value.
>
> If these allocations do genuinely get leaked in some code path,
> this report is of exactly zero help in finding where; and I'm
> afraid I'm not very motivated to go looking for a bug that probably
> doesn't exist.
>
Hi Tom,
thanks for taking a look at this.

I tried to find where the zone table is freed, without success.
It would be a big surprise for me, if this tool is buggy.
Anyway, it's just a sample of the total report, which is 10 mb
(postmaster.log), done with the regression tests.

regards,
Ranier Vilela

Re: Deprecating postfix and factorial operators in PostgreSQL 13

[Tom Lane]

Mark Dilger  writes:
> The deprecation warnings included in this patch warn that postfix operator 
> support, along with both postfix ! and prefix !! factorial operators, will be 
> removed in PostgreSQL 14.

The operator docs should say "use factorial() instead", or words to
that effect.

regards, tom lane

Deprecating postfix and factorial operators in PostgreSQL 13

[Mark Dilger]

Hackers,

Over in [1] we have been discussing the deprecation of postfix operators, with 
the general consensus that deprecation warnings should be included in this 
upcoming release and postfix operator support should be removed in PostgreSQL 
14.  Since not all people who follow -hackers will necessarily have been 
following that thread, I am creating this new thread, with a patch which adds 
the deprecation notices, for your consideration.
 
The only postfix operator we ship in the standard catalogs is the factorial 
operator (!), which is deprecated by this patch.

The standard catalogs also include a prefix factorial operator (!!) which has 
been deprecated since 2011.

The deprecation warnings included in this patch warn that postfix operator 
support, along with both postfix ! and prefix !! factorial operators, will be 
removed in PostgreSQL 14.

Some of the deprecation verbiage supplied by John Naylor, some by me:



v1-0001-Adding-deprecation-notices.patch
Description: Binary data


[1] 
https://www.postgresql.org/message-id/flat/20200519145449.GN13712%40tamriel.snowman.net#34294baed8757c8e078550b9ba80081d

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: [POC]Enable tuple change partition caused by BEFORE TRIGGER

[Alvaro Herrera]

On 2020-Aug-27, Ashutosh Bapat wrote:

> On Wed, 26 Aug 2020 at 22:47, Alvaro Herrera 
> wrote:

> >   But I'm not 100% about running the BEFORE triggers.  Maybe
> > one way to address this is to check whether the BEFORE triggers in the
> > new target partition are clones; if so then they would have run in the
> > original target partition and so must not be run, but otherwise they
> > have to.
> 
> This will work as long as the two BEFORE ROW triggers have the same effect.
> Consider two situations resulting in inserting identical rows 1. row that
> the before row trigger has redirected to a new partition, say part2 2. a
> row inserted directly into the part2 - if both these rows are identical
> before the BEFORE ROW triggers have been applied, they should remain
> identical while inserting into part2. Any divergence might be problematic
> for the application.

Well, that's why I talk about the trigger being "clones" -- with that
term, I mean that their definitions have been inherited from a
definition in some ancestor partitioned table, and so they must be
identical in the partitions.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [PATCH] Covering SPGiST index

[Pavel Borisov]

>
> 3) I didn't quite get the meaning of the assertion, that is added in a few
> places:
>  Assert(so->state.includeTupdesc->natts);
> Should it be Assert(so->state.includeTupdesc->natts > 1) ?
>
It is rather Assert(so->state.includeTupdesc->natts > 0)  as INCLUDE tuple
descriptor should not be initialized and filled in case of index without
INCLUDE attributes and doesn't contain any info about key attribute which
is processed by SpGist existing way separately for different SpGist tuple
types i.e. leaf, prefix=inner and label tuples. So only INCLUDE attributes
are counted there. This and similar Asserts are for the case includeTupdesc
becomes mistakenly initialized by some future code change.

I completely agree with all the other suggestions and made corrections (see
v8). Thank you very much for your review!
Also there is a separate patch 0002 to add VACUUM ANALYZE to
index_including test which is not necessary for covering spgist.

One more point to note: in spgist_private.h I needed to shift down whole
block between
*"typedef struct SpGistSearchItem"*
*and *
*"} SpGistCache;"*
to position it below tuples types declarations to insert pointer
"SpGistLeafTuple leafTuple"; into struct SpGistSearchItem. This is the only
change in this block and I apologize for possible inconvenience to review
this change.

-- 
Best regards,
Pavel Borisov

Postgres Professional: http://postgrespro.com 


v8-0001-Covering-SP-GiST-index-support-for-INCLUDE-column.patch
Description: Binary data


v1-0002-Add-VACUUM-ANALYZE-to-index-including-test.patch
Description: Binary data

Re: Should we replace the checks for access method OID with handler OID?

[Robert Haas]

On Thu, Aug 27, 2020 at 5:37 AM Ashutosh Sharma  wrote:
> While reviewing the patch for pg_surgery contrib module - [1], Asim
> Praveen suggested that it would be better to replace the check for
> access method OID with handler OID. Otherwise, if someone creates a
> new AM using the AM handler that is originally supported for e.g.
> "heap_tableam_handler" and if this new AM is used to create a table,
> then one cannot perform surgery on such tables because we have checks
> for access method OID which would reject this new AM as we only allow
> heap AM. For e.g. if we do this:
>
> create access method myam type table handler heap_tableam_handler;
> create table mytable (…) using myam;
>
> And use an access method OID check, we won't be able to perform
> surgery on mytable created above because it isn't the heap table
> although its table structure is actually heap.

The only reason I can see why it would make sense to do this sort of
thing is if you wanted to create a new AM for testing purposes which
behaves like some existing AM but is technically a different AM. And
if you did that, then I guess the change you are proposing would make
it behave more like it's the same thing after all, which seems like it
might be missing the point.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: ALTER TABLE .. DETACH PARTITION CONCURRENTLY

[Robert Haas]

On Wed, Aug 26, 2020 at 7:40 PM Alvaro Herrera  wrote:
> To mark it detached means to set pg_inherits.inhdetached=true.  That
> column name is a bit of a misnomer, since that column really means "is
> in the process of being detached"; the pg_inherits row goes away
> entirely once the detach process completes.  This mark guarantees that
> everyone will see that row because the detaching session waits long
> enough after committing the first transaction and before deleting the
> pg_inherits row, until everyone else has moved on.

OK. Do you just wait until the XID of the transaction that set
inhdetached is all-visible, or how do you do it?

> The other point is that the partition directory code can be asked to
> include detached partitions, or not to.  The executor does the former,
> and the planner does the latter.  This allows a transient period during
> which the partition descriptor returned to planner and executor is
> different; this makes the situation equivalent to what would have
> happened if the partition was attached during the operation: in executor
> we would detect that there is an additional partition that was not seen
> by the planner, and we already know how to deal with that situation by
> your handling of the ATTACH code.

Ah ha! That is quite clever and I don't think that I would have
thought of it. So all the plans that were created before you set
inhdetached=true have to be guaranteed to be invaliated or gone
altogether before you can actually delete the pg_inherits row. It
seems like it ought to be possible to ensure that, though I am not
surely of the details exactly. Is it sufficient to wait for all
transactions that have locked the table to go away? I'm not sure
exactly how this stuff interacts with the plan cache.

> There is one fly in the ointment though, which is that if you cancel the
> wait and then immediately do the ALTER TABLE DETACH FINALIZE without
> waiting for as long as the original execution would have waited, you
> might end up killing the partition ahead of time.  One solution to this
> would be to cause the FINALIZE action to wait again at start.  This
> would cause it to take even longer, but it would be safer.  (If you
> don't want it to take longer, you can just not cancel it in the first
> place.)  This is not a problem if the server crashes in between (which
> is the scenario I had in mind when doing the FINALIZE thing), because of
> course no transaction can continue to run across a crash.

Yeah, it sounds like this will require some solution, but I agree that
just waiting "longer" seems acceptable.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: Clang Address Sanitizer (Postgres14) Detected Memory Leaks

[Tom Lane]

Ranier Vilela  writes:
> Is this something to worry about, or is it another problem with the
> analysis tool, that nobody cares about?

As far as the first one goes, I'd bet on buggy analysis tool.
The complained-of allocation is evidently for the "extra" state
associated with the timezone GUC variable, and AFAICS guc.c is
quite careful not to leak those.  It is true that the block will
still be allocated at process exit, but that doesn't make it a leak.

I did not trace the second one in any detail, but I don't believe
guc.c leaks sourcefile strings either.  There's only one place
where it overwrites them, and that place frees the old value.

If these allocations do genuinely get leaked in some code path,
this report is of exactly zero help in finding where; and I'm
afraid I'm not very motivated to go looking for a bug that probably
doesn't exist.

regards, tom lane

Re: Add header support to text format and matching feature

[Rémi Lapeyre]

Thanks Daniel for the review and Vignesh for addressing the comments.

I have two remarks with the state of the current patches:
- DefGetCopyHeader() duplicates a lot of code from defGetBoolean(), should we 
refactor this so that they can share more of their internals? In the current 
implementation any change to defGetBoolean() should be made to 
DefGetCopyHeader() too or their behaviour will subtly differ.
- It is possible to set the header option multiple time:
 \copy x(i) from file.txt with (format csv, header off, header on);
  In which case the last one is the one kept. I think this is a bug and it 
should be fixed, but this is already the behaviour in the current 
implementation so fixing it would not be backward compatible. Do you think 
users should not do this and I can fix it or that keeping the current behaviour 
is better for backward compatibility?

Regards,
Rémi

> Le 17 août 2020 à 14:49, vignesh C  a écrit :
> 
> Thanks for your comments, Please find my thoughts inline.
> 
>> In my tests it works fine except for one crash that I can reproduce
>> on a fresh build and default configuration with:
>> 
>> $ cat >file.txt
>> i
>> 1
>> 
>> $ psql
>> postgres=# create table x(i int);
>> CREATE TABLE
>> postgres=# \copy x(i) from file.txt with (header match)
>> COPY 1
>> postgres=# \copy x(i) from file.txt with (header match)
>> COPY 1
>> postgres=# \copy x(i) from file.txt with (header match)
>> COPY 1
>> postgres=# \copy x(i) from file.txt with (header match)
>> COPY 1
>> postgres=# \copy x(i) from file.txt with (header match)
>> COPY 1
>> postgres=# \copy x(i) from file.txt with (header match)
>> PANIC:  ERRORDATA_STACK_SIZE exceeded
>> server closed the connection unexpectedly
>>This probably means the server terminated abnormally
>>before or while processing the request.
>> The connection to the server was lost. Attempting reset: Failed.
>> 
> 
> Fixed, replaced PG_TRY/PG_CATCH with strcmp logic to get the header option.
> 
>> 
>> Code comments:
>> 
>> 
>> +/*
>> + * Represents whether the header must be absent, present or present and
>> match.
>> + */
>> +typedef enum CopyHeader
>> +{
>> +   COPY_HEADER_ABSENT,
>> +   COPY_HEADER_PRESENT,
>> +   COPY_HEADER_MATCH
>> +} CopyHeader;
>> +
>> /*
>>  * This struct contains all the state variables used throughout a COPY
>>  * operation. For simplicity, we use the same struct for all variants of
>> COPY,
>> @@ -136,7 +146,7 @@ typedef struct CopyStateData
>>boolbinary; /* binary format? */
>>boolfreeze; /* freeze rows on loading? */
>>boolcsv_mode;   /* Comma Separated Value
>> format? */
>> -   boolheader_line;/* CSV or text header line? */
>> +   CopyHeader  header_line;/* CSV or text header line? */
>> 
>> 
>> After the redefinition into this enum type, there are still a
>> bunch of references to header_line that treat it like a boolean:
>> 
>> 1190:   if (cstate->header_line)
>> 1398:   if (cstate->binary && cstate->header_line)
>> 2119:   if (cstate->header_line)
>> 3635:   if (cstate->cur_lineno == 0 && cstate->header_line)
>> 
>> It works fine since COPY_HEADER_ABSENT is 0 as the first value of the enum,
>> but maybe it's not good style to count on that.
> 
> Fixed. Changed it to cstate->header_line != COPY_HEADER_ABSENT.
> 
>> 
>> 
>> 
>> +   PG_TRY();
>> +   {
>> +   if (defGetBoolean(defel))
>> +   cstate->header_line =
>> COPY_HEADER_PRESENT;
>> +   else
>> +   cstate->header_line =
>> COPY_HEADER_ABSENT;
>> +   }
>> +   PG_CATCH();
>> +   {
>> +   char*sval = defGetString(defel);
>> +
>> +   if (!cstate->is_copy_from)
>> +   PG_RE_THROW();
>> +
>> +   if (pg_strcasecmp(sval, "match") == 0)
>> +   cstate->header_line =
>> COPY_HEADER_MATCH;
>> +   else
>> +   ereport(ERROR,
>> +
>> (errcode(ERRCODE_SYNTAX_ERROR),
>> +errmsg("header requires a
>> boolean or \"match\"")));
>> +   }
>> +   PG_END_TRY();
>> 
>> It seems wrong to use a PG_CATCH block for this. I understand that
>> it's because defGetBoolean() calls ereport() on non-booleans, but then
>> it should be split into an error-throwing function and a
>> non-error-throwing lexical analysis of the boolean, the above code
>> calling the latter.
>> Besides the comments in elog.h above PG_TRY say that
>> "the error recovery code
>>  can either do PG_RE_THROW to propagate 

Re: factorial function/phase out postfix operators?

[Robert Haas]

On Thu, Aug 27, 2020 at 10:04 AM Tom Lane  wrote:
> Well, the !! operator itself has been "deprecated" for a long time:
>
> regression=# \do+ !!
>  List of operators
>Schema   | Name | Left arg type | Right arg type | Result type |  Function 
>   |Description
> +--+---++-+-+---
>  pg_catalog | !!   |   | bigint | numeric | 
> numeric_fac | deprecated, use ! instead
>  pg_catalog | !!   |   | tsquery| tsquery | 
> tsquery_not | NOT tsquery
> (2 rows)
>
> I'm a bit inclined to kill them both off and standardize on factorial()
> (not numeric_fac).

Works for me. !! hasn't been marked as deprecated in the
documentation, only the operator comment, which probably not many
people look at. But I don't see a problem updating the documentation
now to say:

- !! is going away, use factorial()
- ! is going away, use factorial()
- postfix operators are going away

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: factorial function/phase out postfix operators?

[Mark Dilger]

> On Aug 27, 2020, at 7:04 AM, Tom Lane  wrote:
> 
> Robert Haas  writes:
>> Yeah, that looks like a good spot. I think we should also add
>> something to the documentation of the factorial operator, mentioning
>> that it will be going away. Perhaps we can advise people to write !!3
>> instead of 3! for forward-compatibility, or maybe we should instead
>> suggest numeric_fac(3).
> 
> Well, the !! operator itself has been "deprecated" for a long time:
> 
> regression=# \do+ !!
> List of operators
>   Schema   | Name | Left arg type | Right arg type | Result type |  Function  
>  |Description
> +--+---++-+-+---
> pg_catalog | !!   |   | bigint | numeric | 
> numeric_fac | deprecated, use ! instead
> pg_catalog | !!   |   | tsquery| tsquery | 
> tsquery_not | NOT tsquery
> (2 rows)
> 
> I'm a bit inclined to kill them both off and standardize on factorial()
> (not numeric_fac).
> 
>   regards, tom lane

Just for historical context, it seems that when you committed 
908ab80286401bb20a519fa7dc7a837631f20369 in 2011, you were choosing one 
operator per underlying proc to be the canonical operator name, and deprecating 
all other operators based on the same proc.  You chose postfix ! as the 
canonical operator for numeric_fac and deprecated prefix !!, but I think I can 
infer from that commit that if postfix ! did not exist, prefix !! would have 
been the canonical operator and would not have been deprecated.

The main reason I did not remove prefix !! in this patch series is that the 
patch is about removing postfix operator support, and so it seemed off topic.  
But if there is general agreement to remove prefix !!, I'll put that in the 
next patch.

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: factorial function/phase out postfix operators?

[Tom Lane]

Robert Haas  writes:
> Yeah, that looks like a good spot. I think we should also add
> something to the documentation of the factorial operator, mentioning
> that it will be going away. Perhaps we can advise people to write !!3
> instead of 3! for forward-compatibility, or maybe we should instead
> suggest numeric_fac(3).

Well, the !! operator itself has been "deprecated" for a long time:

regression=# \do+ !!
 List of operators
   Schema   | Name | Left arg type | Right arg type | Result type |  Function   
|Description
+--+---++-+-+---
 pg_catalog | !!   |   | bigint | numeric | numeric_fac 
| deprecated, use ! instead
 pg_catalog | !!   |   | tsquery| tsquery | tsquery_not 
| NOT tsquery
(2 rows)

I'm a bit inclined to kill them both off and standardize on factorial()
(not numeric_fac).

regards, tom lane

Re: Support for OUT parameters in procedures

[Robert Haas]

On Thu, Aug 27, 2020 at 4:34 AM Peter Eisentraut
 wrote:
> For a top-level direct call, you can pass whatever you want, since all
> OUT parameters are presented as initially NULL to the procedure code.
> So you could just pass NULL, as in CALL test_proc(5, NULL).

Is that actually how other systems work? I would think that people
would expect to pass, say, a package variable, and expect that it will
get updated.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: [EXTERNAL] Re: WIP: WAL prefetch (another approach)

[Stephen Frost]

Greetings,

* Sait Talha Nisanci (sait.nisa...@microsoft.com) wrote:
> I have run some benchmarks for this patch. Overall it seems that there is a 
> good improvement with the patch on recovery times:

Maybe I missed it somewhere, but what's the OS/filesystem being used
here..?  What's the filesystem block size..?

Thanks,

Stephen


signature.asc
Description: PGP signature

Re: [EXTERNAL] Re: WIP: WAL prefetch (another approach)

[Robert Haas]

On Wed, Aug 26, 2020 at 9:42 AM Sait Talha Nisanci
 wrote:
> I have tried combination of SSD, HDD, full_page_writes = on/off and 
> max_io_concurrency = 10/50, the recovery times are as follows (in seconds):
>
>No prefetch  | Default prefetch 
> values  |  Default + max_io_concurrency = 50
> SSD, full_page_writes = on  852 301   
>   197
> SSD, full_page_writes = off 16421359  
>   1391
> HDD, full_page_writes = on  60276345  
>   6390
> HDD, full_page_writes = off 738 275   
>   192

The regression on HDD with full_page_writes=on is interesting. I don't
know why that should happen, and I wonder if there is anything that
can be done to mitigate it.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: factorial function/phase out postfix operators?

[Robert Haas]

On Thu, Aug 27, 2020 at 7:12 AM John Naylor  wrote:
> Well, for starters it'll say the obvious, but since we have a concrete
> timeframe, maybe a  tag to make it more visible, like in the
> attached, compressed to avoid confusing the cfbot.

Yeah, that looks like a good spot. I think we should also add
something to the documentation of the factorial operator, mentioning
that it will be going away. Perhaps we can advise people to write !!3
instead of 3! for forward-compatibility, or maybe we should instead
suggest numeric_fac(3).

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

PATCH: Attempt to make dbsize a bit more consistent

[gkokolatos]

Hi all,

this minor patch is attempting to force the use of the tableam api in dbsize 
where ever it is required.

Apparently something similar was introduced for toast relations only. 
Intuitively it seems that the distinction between a table and a toast table is 
not needed. This patch treats tables, toast tables and materialized views 
equally.

Regards,
//Georgios

0001-Attempt-to-make-dbsize-a-bit-more-consistent.patch
Description: Binary data

Re: Please help for error ( file is required for XML support )

[Ashutosh Sharma]

On Thu, Aug 27, 2020 at 6:30 PM Sachin Khanna  wrote:
>
> Hi
>
>
>
> I am new to postgreSQL , I am trying to install the same with XML support but 
> it is giving below error on configuration.
>
>
>
> ./configure --prefix=/opt/postgresql-12.3/pqsql --with-libxml 
> --datadir=/home/postgres/ --with-includes=/usr/lib64/
>

It seems like your include path is pointing to  "/usr/lib64/" which
basically contains the libraries and not the header files, I guess. To
include libraries you should be using --with-libraries option.

Also, I feel that these types of questions are not for hackers
mailing-list. These are configuration related issues and should
probably be raised in pgsql-general mailing list
(pgsql-general.postgresql.org).

-- 
With Regards,
Ashutosh Sharma
EnterpriseDB:http://www.enterprisedb.com

re: Please help for error ( file is required for XML support )

[Ranier Vilela]

>checking for libxml/parser.h... no
>configure: error: header file  is required for XML support

sudo yum install libxml2-devel

You need to install the development package before ./configure.

regards,
Ranier Vilela

Re: list of extended statistics on psql

[Julien Rouhaud]

Hi Yamada-san,

On Thu, Aug 27, 2020 at 03:13:09PM +0900, Tatsuro Yamada wrote:
> 
> I re-read a help message of \d* commands and realized it's better to
> use "\dX".
> There are already cases where the commands differ due to differences
> in case, so I did the same way. Please find attached patch. :-D
> For example:
> ==
>   \da[S]  [PATTERN]  list aggregates
>   \dA[+]  [PATTERN]  list access methods
> ==
> 
> Attached patch uses "\dX" instead of "\dz":
> ==
>   \dx[+]  [PATTERN]  list extensions
>   \dX [PATTERN]  list extended statistics
> ==


Thanks for updating the patch!  This alias will probably be easier to remember.


> 
> Results of regress test of the feature are the following:
> ==
> -- check printing info about extended statistics
> create table t1 (a int, b int);
> create statistics stts_1 (dependencies) on a, b from t1;
> create statistics stts_2 (dependencies, ndistinct) on a, b from t1;
> create statistics stts_3 (dependencies, ndistinct, mcv) on a, b from t1;
> create table t2 (a int, b int, c int);
> create statistics stts_4 on b, c from t2;
> create table hoge (col1 int, col2 int, col3 int);
> create statistics stts_hoge on col1, col2, col3 from hoge;
> 
> \dX
>   List of extended statistics
>  Schema | Table |   Name| Columns  | Ndistinct | Dependencies | 
> MCV
> +---+---+--+---+--+-
>  public | hoge  | stts_hoge | col1, col2, col3 | t | t| t
>  public | t1| stts_1| a, b | f | t| f
>  public | t1| stts_2| a, b | t | t| f
>  public | t1| stts_3| a, b | t | t| t
>  public | t2| stts_4| b, c | t | t| t
> (5 rows)
> 
> \dX stts_?
> List of extended statistics
>  Schema | Table |  Name  | Columns | Ndistinct | Dependencies | MCV
> +---++-+---+--+-
>  public | t1| stts_1 | a, b| f | t| f
>  public | t1| stts_2 | a, b| t | t| f
>  public | t1| stts_3 | a, b| t | t| t
>  public | t2| stts_4 | b, c| t | t| t
> (4 rows)
> 
> \dX *hoge
>   List of extended statistics
>  Schema | Table |   Name| Columns  | Ndistinct | Dependencies | 
> MCV
> +---+---+--+---+--+-
>  public | hoge  | stts_hoge | col1, col2, col3 | t | t| t
> (1 row)
> ==


Thanks also for the documentation and regression tests.  This overall looks
good, I just have a two comments:

- there's a whitespace issue in the documentation part:

add_list_extended_stats_for_psql_by_dX_command.patch:10: tab in indent.
  
warning: 1 line adds whitespace errors.

- You're sorting the output on schema, table, extended statistics and columns
  but I think the last one isn't required since extended statistics names are
  unique.

history file on replica and double switchover

[Grigory Smolkin]

Hello!

I`ve noticed, that when running switchover replica to master and back to 
replica, new history file is streamed to replica, but not archived,
which is not great, because it breaks PITR if archiving is running on 
replica. The fix looks trivial.

Bash script to reproduce the problem and patch are attached.

--
Grigory Smolkin
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

diff --git a/src/backend/replication/walreceiver.c b/src/backend/replication/walreceiver.c
index 7c11e1ab44..63a3d0c1e6 100644
--- a/src/backend/replication/walreceiver.c
+++ b/src/backend/replication/walreceiver.c
@@ -761,6 +761,10 @@ WalRcvFetchTimeLineHistoryFiles(TimeLineID first, TimeLineID last)
 */
writeTimeLineHistoryFile(tli, content, len);
 
+   /* Mark history file as ready for archiving */
+   if (XLogArchiveMode != ARCHIVE_MODE_OFF)
+   XLogArchiveNotify(fname);
+
pfree(fname);
pfree(content);
}


double_switchover.sh
Description: application/shellscript

Please help for error ( file is required for XML support )

[Sachin Khanna]

Hi

I am new to postgreSQL , I am trying to install the same with XML support but 
it is giving below error on configuration.

./configure --prefix=/opt/postgresql-12.3/pqsql --with-libxml 
--datadir=/home/postgres/ --with-includes=/usr/lib64/

checking for libxml/parser.h... no
configure: error: header file  is required for XML support



I am using below RHEL flavor and have installed below xml package for support

  Operating System: Red Hat Enterprise Linux Server 7.9 Beta (Maipo)
   CPE OS Name: cpe:/o:redhat:enterprise_linux:7.9:beta:server
Kernel: Linux 3.10.0-1136.el7.ppc64le
  Architecture: ppc64-le


rpm -qi libxml2-2.9.1-6.el7.4.ppc64le
Name: libxml2
Version : 2.9.1
Release : 6.el7.4
Architecture: ppc64le
Install Date: Thu 09 Jul 2020 02:55:33 PM CDT
Group   : Development/Libraries
Size: 2594518
License : MIT
Signature   : RSA/SHA256, Tue 26 Nov 2019 08:05:06 AM CST, Key ID 
199e2f91fd431d51
Source RPM  : libxml2-2.9.1-6.el7.4.src.rpm
Build Date  : Tue 26 Nov 2019 07:21:34 AM CST
Build Host  : ppc-029.build.eng.bos.redhat.com
Relocations : (not relocatable)
Packager: Red Hat, Inc. 
Vendor  : Red Hat, Inc.
URL : http://xmlsoft.org/
Summary : Library providing XML and HTML support
Description :
This library allows to manipulate XML files. It includes support
to read, modify and write XML and HTML files. There is DTDs support
this includes parsing and validation even with complex DtDs, either
at parse time or later once the document has been modified. The output
can be a simple SAX stream or and in-memory DOM like representations.
In this case one can use the built-in XPath and XPointer implementation
to select sub nodes or ranges. A flexible Input/Output mechanism is
available, with existing HTTP and FTP modules and combined to an
URI library.


ls -ltr /usr/lib64/ | grep -i xml
-rwxr-xr-x.  1 root root68528 May 19  2015 libxmlrpc_util.so.3.32
-rwxr-xr-x.  1 root root   136488 May 19  2015 libxmlrpc.so.3.32
-rwxr-xr-x.  1 root root68488 May 19  2015 libxmlrpc_server.so.3.32
-rwxr-xr-x.  1 root root68336 May 19  2015 libxmlrpc_server_cgi.so.3.32
-rwxr-xr-x.  1 root root68416 May 19  2015 libxmlrpc_server_abyss.so.3.32
-rwxr-xr-x.  1 root root69720 May 19  2015 libxmlrpc_client.so.3.32
-rwxr-xr-x.  1 root root   138240 May 19  2015 libxmlrpc_abyss.so.3.32
-rwxr-xr-x.  1 root root  2271136 Nov 26  2019 libxml2.so.2.9.1
-rwxr-xr-x.  1 root root   403952 Dec  6  2019 libQtXml.so.4.8.7
-rwxr-xr-x.  1 root root  5621696 Dec  6  2019 libQtXmlPatterns.so.4.8.7
-rwxr-xr-x.  1 root root   336816 Feb 19  2020 libQt5Xml.so.5.9.7
lrwxrwxrwx.  1 root root   16 Jul  9 14:55 libxml2.so.2 -> libxml2.so.2.9.1
lrwxrwxrwx.  1 root root   17 Jul  9 14:55 libxmlrpc.so.3 -> 
libxmlrpc.so.3.32
lrwxrwxrwx.  1 root root   23 Jul  9 14:55 libxmlrpc_abyss.so.3 -> 
libxmlrpc_abyss.so.3.32
lrwxrwxrwx.  1 root root   22 Jul  9 14:55 libxmlrpc_util.so.3 -> 
libxmlrpc_util.so.3.32
lrwxrwxrwx.  1 root root   24 Jul  9 14:55 libxmlrpc_server.so.3 -> 
libxmlrpc_server.so.3.32
lrwxrwxrwx.  1 root root   28 Jul  9 14:55 libxmlrpc_server_cgi.so.3 -> 
libxmlrpc_server_cgi.so.3.32
lrwxrwxrwx.  1 root root   30 Jul  9 14:55 libxmlrpc_server_abyss.so.3 -> 
libxmlrpc_server_abyss.so.3.32
lrwxrwxrwx.  1 root root   24 Jul  9 14:56 libxmlrpc_client.so.3 -> 
libxmlrpc_client.so.3.32
lrwxrwxrwx.  1 root root   18 Jul  9 14:56 libQt5Xml.so.5.9 -> 
libQt5Xml.so.5.9.7
lrwxrwxrwx.  1 root root   18 Jul  9 14:56 libQt5Xml.so.5 -> 
libQt5Xml.so.5.9.7
lrwxrwxrwx.  1 root root   17 Jul 21 15:51 libQtXml.so.4.8 -> 
libQtXml.so.4.8.7
lrwxrwxrwx.  1 root root   17 Jul 21 15:51 libQtXml.so.4 -> 
libQtXml.so.4.8.7
lrwxrwxrwx.  1 root root   25 Jul 21 15:51 libQtXmlPatterns.so.4.8 -> 
libQtXmlPatterns.so.4.8.7
lrwxrwxrwx.  1 root root   25 Jul 21 15:51 libQtXmlPatterns.so.4 -> 
libQtXmlPatterns.so.4.8.7
lrwxrwxrwx.  1 root root   27 Aug 27 02:52 libxml2.so -> 
/usr/lib64/libxml2.so.2.9.1


Thanks and Regards,
SACHIN KHANNA
212 BASIS DBA TEAM OFFSHORE
Office : 204058624
Cell : 9049522511
Email: sachin.kha...@sysco.com
Infosys Technologies Limited (r) | PUNE

RE: Help needed configuring postgreSQL with xml support

[Sachin Khanna]

Thanks for the quick response. I am using RED HAT linux ( ppc64-le ).


  Operating System: Red Hat Enterprise Linux Server 7.9 Beta (Maipo)
   CPE OS Name: cpe:/o:redhat:enterprise_linux:7.9:beta:server
Kernel: Linux 3.10.0-1136.el7.ppc64le
  Architecture: ppc64-le
You have new mail in /var/spool/mail/root

I have gone into the details given for xml support and have installed the 
required rpm and tried to setup the env variables as well like 

export XML2_CFLAGS='/usr/lib64/'
export XML2_CONFIG='/usr/lib64/'
export XML2_LIBS='/usr/lib64/libxml2.so.2.9.1'

rpm -qi libxml2-2.9.1-6.el7.4.ppc64le
Name: libxml2
Version : 2.9.1
Release : 6.el7.4
Architecture: ppc64le
Install Date: Thu 09 Jul 2020 02:55:33 PM CDT
Group   : Development/Libraries
Size: 2594518

yum list installed |grep xml
libxml2.ppc64le 2.9.1-6.el7.4  @anaconda/7.9
libxml2-python.ppc64le  2.9.1-6.el7.4  @anaconda/7.9
python-lxml.ppc64le 3.2.1-4.el7@anaconda/7.9
xml-common.noarch   0.6.3-39.el7   @anaconda/7.9
xml2.ppc64le0.5-7.el7  @epel
xmlrpc-c.ppc64le1.32.5-1905.svn2451.el7@anaconda/7.9
xmlrpc-c-client.ppc64le 1.32.5-1905.svn2451.el7@anaconda/7.9


--with-libxml
Build with libxml2, enabling SQL/XML support. Libxml2 version 2.6.23 or later 
is required for this feature.

To detect the required compiler and linker options, PostgreSQL will query 
pkg-config, if that is installed and knows about libxml2. Otherwise the program 
xml2-config, which is installed by libxml2, will be used if it is found. Use of 
pkg-config is preferred, because it can deal with multi-architecture 
installations better.

To use a libxml2 installation that is in an unusual location, you can set 
pkg-config-related environment variables (see its documentation), or set the 
environment variable XML2_CONFIG to point to the xml2-config program belonging 
to the libxml2 installation, or set the variables XML2_CFLAGS and XML2_LIBS. 
(If pkg-config is installed, then to override its idea of where libxml2 is you 
must either set XML2_CONFIG or set both XML2_CFLAGS and XML2_LIBS to nonempty 
strings.)


Thanks and Regards,
SACHIN KHANNA
212 BASIS DBA TEAM OFFSHORE
Office : 204058624
Cell : 9049522511

-Original Message-
From: Ashutosh Sharma  
Sent: Thursday, August 27, 2020 2:12 PM
To: Khanna, Sachin 000 
Cc: pgsql-hack...@postgresql.org; Thomas Munro 
Subject: Re: Help needed configuring postgreSQL with xml support

EXTERNAL EMAIL: This email originated from outside of the organization. Do not 
click links or open attachments unless you recognize the sender and know the 
content is safe

In addition to what Thomas said, I would also recommend you to refer to the 
description of --with-libxml command line option provided in the postgres 
installation-procedure page - [1].

[1] - 
https://urldefense.proofpoint.com/v2/url?u=https-3A__www.postgresql.org_docs_12_install-2Dprocedure.html&d=DwIBaQ&c=Iej4I5bEYPmgv5l2sS6i8A&r=RbbcFo0QGVsZI41vIxHdvJHDEomAuf-kGUMMC7Yn2sE&m=D5QMYkp-FOKiI-htZxXqsxkhULT3OKhPaZbElnB9CWY&s=oDlVjDkRb48Sqx0fUgv6hoehucADn74yxrut4uiL50E&e=

--
With Regards,
Ashutosh Sharma
EnterpriseDB:https://urldefense.proofpoint.com/v2/url?u=http-3A__www.enterprisedb.com&d=DwIBaQ&c=Iej4I5bEYPmgv5l2sS6i8A&r=RbbcFo0QGVsZI41vIxHdvJHDEomAuf-kGUMMC7Yn2sE&m=D5QMYkp-FOKiI-htZxXqsxkhULT3OKhPaZbElnB9CWY&s=vhT8MXXxy_b8UWkK9HhFKfoRwA8BRWxLEk0s0QcCk_I&e=

On Thu, Aug 27, 2020 at 1:56 PM Thomas Munro  wrote:
>
> On Thu, Aug 27, 2020 at 8:17 PM Khanna, Sachin 000 
>  wrote:
> > I am getting following error in  configuration.log of installation . 
> > Please help
>
> You didn't mention what operating system this is, but, for example, if 
> it's Debian, Ubuntu or similar you might need to install libxml2-dev 
> and pkg-config for --with-libxml to work.
>
>

Evaluate expression at planning time for two more cases

[Surafel Temesgen]

Hi,

In good written query IS NULL and IS NOT NULL check on primary and non null
constraints columns should not happen but if it is mentioned PostgreSQL
have to be smart enough for not checking every return result about null
value on primary key column. Instead it can be evaluate its truth value and
set the result only once. The attached patch evaluate and set the truth
value for null and not null check on primary column on planning time if the
relation attribute is not mention on nullable side of outer join.

Thought?

regards

Surafel
diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
index 750586fceb..417758f705 100644
--- a/src/backend/optimizer/util/clauses.c
+++ b/src/backend/optimizer/util/clauses.c
@@ -22,6 +22,7 @@
 #include "access/htup_details.h"
 #include "catalog/pg_aggregate.h"
 #include "catalog/pg_class.h"
+#include "catalog/pg_constraint.h"
 #include "catalog/pg_language.h"
 #include "catalog/pg_operator.h"
 #include "catalog/pg_proc.h"
@@ -42,6 +43,7 @@
 #include "parser/parse_agg.h"
 #include "parser/parse_coerce.h"
 #include "parser/parse_func.h"
+#include "parser/parsetree.h"
 #include "rewrite/rewriteManip.h"
 #include "tcop/tcopprot.h"
 #include "utils/acl.h"
@@ -90,6 +92,14 @@ typedef struct
 	char	   *prosrc;
 } inline_error_callback_arg;
 
+typedef struct check_null_side_state
+{
+	Relids		relids;			/* base relids within this subtree */
+	bool		contains_outer; /* does subtree contain outer join(s)? */
+	JoinType	jointype;		/* type of join */
+	List	   *sub_states;		/* List of states for subtree components */
+}			check_null_side_state;
+
 typedef struct
 {
 	char		max_hazard;		/* worst proparallel hazard found so far */
@@ -156,6 +166,9 @@ static Query *substitute_actual_srf_parameters(Query *expr,
 			   int nargs, List *args);
 static Node *substitute_actual_srf_parameters_mutator(Node *node,
 	  substitute_actual_srf_parameters_context *context);
+static bool check_null_side(PlannerInfo *root, int relid);
+static check_null_side_state * collect_jointree_data(Node *jtnode);
+
 
 
 /*
@@ -2245,7 +2258,6 @@ rowtype_field_matches(Oid rowtypeid, int fieldnum,
 	return true;
 }
 
-
 /*
  * eval_const_expressions
  *
@@ -2626,6 +2638,7 @@ eval_const_expressions_mutator(Node *node,
 	{
 		has_null_input |= ((Const *) lfirst(arg))->constisnull;
 		all_null_input &= ((Const *) lfirst(arg))->constisnull;
+
 	}
 	else
 		has_nonconst_input = true;
@@ -3382,7 +3395,47 @@ eval_const_expressions_mutator(Node *node,
 
 	return makeBoolConst(result, false);
 }
+if (IsA(arg, Var) &&
+	((Var *) arg)->varlevelsup == 0 && context->root)
+{
+	/*
+	 * Evaluate the test if it is on primary column and the
+	 * relation is not mentioned on nullable side of outer
+	 * join
+	 */
+	Var		   *var = (Var *) arg;
+	Query	   *parse = context->root->parse;
+	int			relid;
+	Bitmapset  *pkattnos;
+	Oid			constraintOid;
+	RangeTblEntry *rte;
 
+	relid = var->varno;
+	rte = rt_fetch(relid, parse->rtable);
+	pkattnos = get_primary_key_attnos(rte->relid, false, &constraintOid);
+
+	if (pkattnos != NULL && bms_is_member(var->varattno - FirstLowInvalidHeapAttributeNumber, pkattnos)
+		&& !check_null_side(context->root, relid))
+	{
+		bool		result;
+
+		switch (ntest->nulltesttype)
+		{
+			case IS_NULL:
+result = false;
+break;
+			case IS_NOT_NULL:
+result = true;
+break;
+			default:
+elog(ERROR, "unrecognized nulltesttype: %d",
+	 (int) ntest->nulltesttype);
+result = false; /* keep compiler quiet */
+break;
+		}
+		return makeBoolConst(result, false);
+	}
+}
 newntest = makeNode(NullTest);
 newntest->arg = (Expr *) arg;
 newntest->nulltesttype = ntest->nulltesttype;
@@ -3572,6 +3625,118 @@ eval_const_expressions_mutator(Node *node,
 	return ece_generic_processing(node);
 }
 
+/*
+ * Check a relation attributes on nullable side of the outer join.
+ */
+static bool
+check_null_side(PlannerInfo *root, int relid)
+{
+	check_null_side_state *state;
+	ListCell   *l;
+
+	state = collect_jointree_data((Node *) root->parse->jointree);
+
+	/* if no outer joins the relation is not on nullable side */
+	if (state == NULL || !state->contains_outer)
+		return false;
+
+	/* scan the state and check relation on nullable outer join side */
+	foreach(l, state->sub_states)
+	{
+		check_null_side_state *sub_state = (check_null_side_state *) lfirst(l);
+
+		if (sub_state->contains_outer)
+		{
+			if (sub_state->jointype == JOIN_LEFT)
+			{
+check_null_side_state *right_state = (check_null_side_state *) lsecond(sub_state->sub_states);
+
+if (bms_is_member(relid, right_state->relids))
+	return true;
+			}
+			if (sub_state->jointype == JOIN_RIGHT)
+		

Re: Parallel copy

[Amit Kapila]

On Thu, Aug 27, 2020 at 4:56 PM vignesh C  wrote:
>
> On Thu, Aug 27, 2020 at 8:24 AM Amit Kapila  wrote:
> >
> > On Thu, Aug 27, 2020 at 8:04 AM Greg Nancarrow  wrote:
> > >
> > > > I have attached new set of patches with the fixes.
> > > > Thoughts?
> > >
> > > Hi Vignesh,
> > >
> > > I don't really have any further comments on the code, but would like
> > > to share some results of some Parallel Copy performance tests I ran
> > > (attached).
> > >
> > > The tests loaded a 5GB CSV data file into a 100 column table (of
> > > different data types). The following were varied as part of the test:
> > > - Number of workers (1 – 10)
> > > - No indexes / 4-indexes
> > > - Default settings / increased resources (shared_buffers,work_mem, etc.)
> > >
> > > (I did not do any partition-related tests as I believe those type of
> > > tests were previously performed)
> > >
> > > I built Postgres (latest OSS code) with the latest Parallel Copy patches 
> > > (v4).
> > > The test system was a 32-core Intel Xeon E5-4650 server with 378GB of RAM.
> > >
> > >
> > > I observed the following trends:
> > > - For the data file size used, Parallel Copy achieved best performance
> > > using about 9 – 10 workers. Larger data files may benefit from using
> > > more workers. However, I couldn’t really see any better performance,
> > > for example, from using 16 workers on a 10GB CSV data file compared to
> > > using 8 workers. Results may also vary depending on machine
> > > characteristics.
> > > - Parallel Copy with 1 worker ran slower than normal Copy in a couple
> > > of cases (I did question if allowing 1 worker was useful in my patch
> > > review).
> >
> > I think the reason is that for 1 worker case there is not much
> > parallelization as a leader doesn't perform the actual load work.
> > Vignesh, can you please once see if the results are reproducible at
> > your end, if so, we can once compare the perf profiles to see why in
> > some cases we get improvement and in other cases not. Based on that we
> > can decide whether to allow the 1 worker case or not.
> >
>
> I will spend some time on this and update.
>

Thanks.

-- 
With Regards,
Amit Kapila.

Re: Dumping/restoring fails on inherited generated column

[Masahiko Sawada]

On Thu, 23 Jul 2020 at 19:55, Masahiko Sawada
 wrote:
>
> On Thu, 16 Jul 2020 at 04:29, Tom Lane  wrote:
> >
> > Peter Eisentraut  writes:
> > >> Right, there were a number of combinations that were not properly
> > >> handled.  The attached patch should fix them all.  It's made against
> > >> PG12 but also works on master.  See contained commit message and
> > >> documentation for details.
> >
> > > committed to master and PG12
> >
> > So ... this did not actually fix the dump/restore problem.  In fact,
> > it's worse, because in HEAD I see two failures not one when doing the
> > same test proposed at the start of this thread:
> >
> > 1. make installcheck
> > 2. pg_dump -Fc regression >r.dump
> > 3. createdb r2
> > 4. pg_restore -d r2 r.dump
> >
> > pg_restore: while PROCESSING TOC:
> > pg_restore: from TOC entry 6253; 2604 226187 DEFAULT gtest1_1 b postgres
> > pg_restore: error: could not execute query: ERROR:  column "b" of relation 
> > "gtest1_1" is a generated column
> > Command was: ALTER TABLE ONLY public.gtest1_1 ALTER COLUMN b SET DEFAULT (a 
> > * 2);
> >
> >
> > pg_restore: from TOC entry 6279; 2604 227276 DEFAULT gtest30_1 b postgres
> > pg_restore: error: could not execute query: ERROR:  cannot use column 
> > reference in DEFAULT expression
> > Command was: ALTER TABLE ONLY public.gtest30_1 ALTER COLUMN b SET DEFAULT 
> > (a * 2);
> >
> >
> > pg_restore: warning: errors ignored on restore: 2
> >
>
> The minimum reproducer is:
>
> create table a (a int, b int generated always as (a * 2) stored);
> create table aa () inherits (a);
>
> pg_dump produces the following DDLs:
>
> CREATE TABLE public.a (
> a integer,
> b integer GENERATED ALWAYS AS ((a * 2)) STORED
> );
>
> CREATE TABLE public.aa (
> )
> INHERITS (public.a);
>
> ALTER TABLE ONLY public.aa ALTER COLUMN b SET DEFAULT (a * 2);
>
> However, the ALTER TABLE fails.
>
> By commit 086ffddf, the child tables must have the same generation
> expression as the expression defined in the parent. So I think pg_dump
> should not generate the last DDL. I've attached the patch fixing this
> issue.
>
> Apart from the fix, I wonder if we can add a test that dumps the
> database where executed 'make check' and restore it to another
> database.
>

This issue is not fixed yet. I've attached the updated version patch
and registered it to commit fest so as not to forget. Please review
it.

Regards,

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


fix_gcolumn_dump_v2.patch
Description: Binary data

Re: Parallel copy

[vignesh C]

On Thu, Aug 27, 2020 at 8:24 AM Amit Kapila  wrote:
>
> On Thu, Aug 27, 2020 at 8:04 AM Greg Nancarrow  wrote:
> >
> > > I have attached new set of patches with the fixes.
> > > Thoughts?
> >
> > Hi Vignesh,
> >
> > I don't really have any further comments on the code, but would like
> > to share some results of some Parallel Copy performance tests I ran
> > (attached).
> >
> > The tests loaded a 5GB CSV data file into a 100 column table (of
> > different data types). The following were varied as part of the test:
> > - Number of workers (1 – 10)
> > - No indexes / 4-indexes
> > - Default settings / increased resources (shared_buffers,work_mem, etc.)
> >
> > (I did not do any partition-related tests as I believe those type of
> > tests were previously performed)
> >
> > I built Postgres (latest OSS code) with the latest Parallel Copy patches 
> > (v4).
> > The test system was a 32-core Intel Xeon E5-4650 server with 378GB of RAM.
> >
> >
> > I observed the following trends:
> > - For the data file size used, Parallel Copy achieved best performance
> > using about 9 – 10 workers. Larger data files may benefit from using
> > more workers. However, I couldn’t really see any better performance,
> > for example, from using 16 workers on a 10GB CSV data file compared to
> > using 8 workers. Results may also vary depending on machine
> > characteristics.
> > - Parallel Copy with 1 worker ran slower than normal Copy in a couple
> > of cases (I did question if allowing 1 worker was useful in my patch
> > review).
>
> I think the reason is that for 1 worker case there is not much
> parallelization as a leader doesn't perform the actual load work.
> Vignesh, can you please once see if the results are reproducible at
> your end, if so, we can once compare the perf profiles to see why in
> some cases we get improvement and in other cases not. Based on that we
> can decide whether to allow the 1 worker case or not.
>

I will spend some time on this and update.

Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com

Re: Strange behavior with polygon and NaN

[Kyotaro Horiguchi]

At Wed, 26 Aug 2020 08:18:49 -0400, Tom Lane  wrote in 
> Kyotaro Horiguchi  writes:
> > At Tue, 25 Aug 2020 19:03:50 -0400, Bruce Momjian  wrote 
> > in 
> >> I can confirm that this two-month old email report still produces
> >> different results with indexes on/off in git master, which I don't think
> >> is ever correct behavior.
> 
> > I agree to that the behavior is broken. 
> 
> Yeah, but ... what is "non broken" in this case?  I'm not convinced
> that having point_inside() return zero for any case involving NaN
> is going to lead to noticeably saner behavior than today.

Yes, just doing that leaves many unfixed behavior come from NaNs, but
at least it seems to me one of sane definition candidates that a point
cannot be inside a polygon when NaN is involved. It's similar to
Fpxx() returns false if NaN is involved. As mentioned, I had't fully
checked and haven't considered this seriously, but I changed my mind
to check all the callers. I started checking all the callers of
point_inside, then finally I had to check all functions in geo_ops.c:(

The attached is the result as of now.

=== Resulting behavior

With the attached:

 - All boolean functions return false if NaN is involved.
 - All float8 functions return NaN if NaN is involved.
 - All geometric arithmetics return NaN as output if NaN is involved.

With some exceptions:
 - line_eq: needs to consider that NaNs are equal each other.
 - point_eq/ne (point_eq_pint): ditto
 - lseg_eq/ne: ditto

The change makes some difference in the regression test.
For example,

 <->  changed from 0 to NaN. (distance)
 <@   changed from true to false. (contained)
 <->  changed from 0 to NaN. (distance)
 ?#   changed from true to false (overlaps)


=== pg_hypot mistake?

I noticed that pg_hypot returns inf for the parameters (NaN, Inf) but
I think NaN is appropriate here since other operators behaves that
way. This change causes a change of distance between point(1e+300,Inf)
and line (1,-1,0) from infinity to NaN, which I think is correct
because the arithmetic generates NaN as an intermediate value.


=== Infinity annoyances

Infinity makes some not-great changes in regresssion results. For example:

- point '(1e+300,Infinity)' <-> path '((10,20))' returns
  NaN(previously Infinity), but point '(1e+300,Infinity)' <-> path
  '[(1,2),(3,4)]' returns Infinity.  The difference of the two
  expressions is whether (0 * Inf = NaN) is performed or not. The
  former performs it but that was concealed by not propagating NaN to
  upper layer without the patch.

- Without the patch, point '(1e+300,Infinity)' ## box '(2,2),(0,0)'
  generates '(0,2)', which is utterly wrong. It is because
  box_closept_point evaluates float8_lt(Inf, NaN) as true(!) and sets
  the wrong point for distance=NaN is set. With the patch, the NaN
  makes the result NULL.

- This is not a difference caused by this patch, but for both patched
  and unpatched, point '(1e+300,Inf)' <-> line '{3,0,0}' returns NaN,
  which should be 1e+300. However, the behavior comes from arithmetic
  reasons and wouldn't be a problem.

create_index.out is changed since point(NaN,NaN) <@ polygon changed
from true to false, which seems rather saner.

I haven't checked unchanged results but at least changed results seems
saner to me.


regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
>From 432a671517e78061edc87d18aec291f5629fcbe6 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi 
Date: Thu, 27 Aug 2020 14:49:21 +0900
Subject: [PATCH v1] Fix NaN handling of some geometric operators and functions

Some geometric operators shows somewhat odd behavior comes from NaN
handling and that leads to inconsistency between index scan and seq
scan on geometric conditions.

For example:
  point '(NaN,NaN)' <-> polygon '((0,0),(0,1),(1,1))' => 0, not NaN
  point '(0.3,0.5)' <-> polygon '((0,0),(0,NaN),(1,1))' => 1.14, not NaN
  point '(NaN,NaN)' <@ polygon '((0,0),(0,1),(1,1))'  => true, not false

Some other functions returned totally wrong result like this:
 point '(1e+300,Infinity)' ## box '(2,2),(0,0)' => '(0,2)'

Fix NaN handling of geo_ops.c so that these generates saner results.
---
 src/backend/utils/adt/geo_ops.c| 183 --
 src/test/regress/expected/create_index.out |   2 +-
 src/test/regress/expected/geometry.out | 209 +
 3 files changed, 248 insertions(+), 146 deletions(-)

diff --git a/src/backend/utils/adt/geo_ops.c b/src/backend/utils/adt/geo_ops.c
index a7db783958..916ae87d05 100644
--- a/src/backend/utils/adt/geo_ops.c
+++ b/src/backend/utils/adt/geo_ops.c
@@ -904,9 +904,19 @@ box_intersect(PG_FUNCTION_ARGS)
 
 	result = (BOX *) palloc(sizeof(BOX));
 
-	result->high.x = float8_min(box1->high.x, box2->high.x);
+	/* float8_min conceals NaN, check separately for NaNs */
+	if (unlikely(isnan(box1->high.x) || isnan(box2->high.x)))
+		result->high.x = get_float8_nan();
+	else
+		result->high.x = float8_min(box1->high.x, box2->high.x);
+
 	result->low.x = float8_max(bo

Re: factorial function/phase out postfix operators?

[John Naylor]

On Wed, Aug 26, 2020 at 8:55 PM Robert Haas  wrote:
>
> On Wed, Aug 26, 2020 at 11:57 AM Mark Dilger
>  wrote:
> > I wonder if we can get more comments for or against this patch, at least in 
> > principle, in the very near future, to help determine whether the 
> > deprecation notices should go into v13?
>
> Speaking of that, has somebody written a specific patch for that?
> Like, exactly what are we proposing that this deprecation warning is
> going to say?

Well, for starters it'll say the obvious, but since we have a concrete
timeframe, maybe a  tag to make it more visible, like in the
attached, compressed to avoid confusing the cfbot.

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


postfix-depr.patch.gz
Description: GNU Zip compressed data

Re: Handing off SLRU fsyncs to the checkpointer

[Thomas Munro]

On Thu, Aug 27, 2020 at 8:48 PM Jakub Wartak  wrote:
> I've tried to get cache misses ratio via PMCs, apparently on EC2 they are 
> (even on bigger) reporting as not-supported or zeros.

I heard some of the counters are only allowed on their dedicated instance types.

> However interestingly the workload has IPC of 1.40 (instruction bound) which 
> to me is strange as I would expect BufTableLookup() to be actually heavy 
> memory bound (?) Maybe I'll try on some different hardware one day.

Hmm, OK now you've made me go and read a bunch of Brendan Gregg bloggs
and try some experiments of my own to get a feel for this number and
what it might be telling us about the cache miss counters you can't
see.  Since I know how to generate arbitrary cache miss workloads for
quick experiments using hash joins of different sizes, I tried that
and noticed that when LLC misses were at 76% (bad), IPC was at 1.69
which is still higher than what you're seeing.  When the hash table
was much smaller and LLC misses were down to 15% (much better), IPC
was at 2.83.  I know Gregg said[1] "An IPC < 1.0 likely means memory
bound, and an IPC > 1.0 likely means instruction bound", but that's
not what I'm seeing here, and in his comments section that is
disputed.  So I'm not sure your IPC of 1.40 is evidence against the
hypothesis on its own.

[1] http://www.brendangregg.com/blog/2017-05-09/cpu-utilization-is-wrong.html

Re: Handing off SLRU fsyncs to the checkpointer

[Jakub Wartak]

Hi Alvaro, Thomas, hackers

>> 14.69%  postgres  postgres[.] hash_search_with_hash_value
>> ---hash_search_with_hash_value
>>|--9.80%--BufTableLookup
[..]
>> --4.90%--smgropen
>>   |--2.86%--ReadBufferWithoutRelcache
> Looking at an earlier report of this problem I was thinking whether it'd
> make sense to replace SMgrRelationHash with a simplehash table; I have a
> half-written patch for that, but I haven't completed that work.
> However, in the older profile things were looking different, as
> hash_search_with_hash_value was taking 35.25%, and smgropen was 33.74%
> of it.  BufTableLookup was also there but only 1.51%.  So I'm not so
> sure now that that'll pay off as clearly as I had hoped.

Yes, quite frankly my expectation was to see 
hash_search_with_hash_value()<-smgropen() outcome as 1st one, but in simplified 
redo-bench script it's not the case. The original scenario was much more 
complex with plenty of differences (in no particular order: TB-sized DB VS 
~500GB RAM -> thousands of forks, multiple tables, huge btrees, multiple 
INSERTs wih plenty of data in VALUES() thrown as one commit, real 
primary->hot-standby replication [not closed DB in recovery], sorted not random 
UUIDs) - I'm going to try nail down these differences and maybe I manage to 
produce more realistic "pgbench reproducer" (this may take some time though).

-Jakub Wartak.

Should we replace the checks for access method OID with handler OID?

[Ashutosh Sharma]

Hi All,

While reviewing the patch for pg_surgery contrib module - [1], Asim
Praveen suggested that it would be better to replace the check for
access method OID with handler OID. Otherwise, if someone creates a
new AM using the AM handler that is originally supported for e.g.
"heap_tableam_handler" and if this new AM is used to create a table,
then one cannot perform surgery on such tables because we have checks
for access method OID which would reject this new AM as we only allow
heap AM. For e.g. if we do this:

create access method myam type table handler heap_tableam_handler;
create table mytable (…) using myam;

And use an access method OID check, we won't be able to perform
surgery on mytable created above because it isn't the heap table
although its table structure is actually heap.

This problem won't be there if the check for access method OID is
replaced with handler OID. I liked this suggestion from Asim and did
the changes accordingly. However, while browsing the code for other
contrib modules, I could find such checks present in some of the
contrib modules like pgstattuple, pageinspect and pgrowlocks as well.
So, just wondering if we should be doing similar changes in these
contrib modules also.

Thoughts?

[1] - 
https://www.postgresql.org/message-id/1D56CEFD-E195-4E6B-B870-3383E3E8C65E%40vmware.com

--
With Regards,
Ashutosh Sharma
EnterpriseDB:http://www.enterprisedb.com

Re: factorial function/phase out postfix operators?

[John Naylor]

On Wed, Aug 26, 2020 at 6:57 PM Mark Dilger
 wrote:
>
>
>
> > On Aug 26, 2020, at 6:33 AM, John Naylor  
> > wrote:
>
> > and since POSTFIXOP is gone
> > it doesn't seem to matter how low it is. In fact, I found that the
> > lines with INDENT and UNBOUNDED now work as the lowest precedence
> > declarations. Maybe that's worth something?
> >
> > Following on Peter E.'s example upthread, GENERATED can be removed
> > from precedence, and I also found the same is true for PRESERVE and
> > STRIP_P.
> >
> > I've attached a patch which applies on top of 0001 to demonstrate
> > this. There might possibly still be syntax errors for things not
> > covered in the regression test, but there are no s/r conflicts at
> > least.
>
> I don't have any problem with the changes you made in your patch, but 
> building on your changes I also found that the following cleanup causes no 
> apparent problems:
>
> -%nonassoc  UNBOUNDED   /* ideally should have same 
> precedence as IDENT */
> -%nonassoc  IDENT PARTITION RANGE ROWS GROUPS PRECEDING FOLLOWING CUBE 
> ROLLUP
> +%nonassoc  UNBOUNDED IDENT
> +%nonassoc  PARTITION RANGE ROWS GROUPS PRECEDING FOLLOWING CUBE ROLLUP
>
> Which does what the old comment apparently wanted.

This changes the context of the comment at the top of the block:

 * To support target_el without AS, we must give IDENT an explicit priority
 * lower than Op.  We can safely assign the same priority to various
 * unreserved keywords as needed to resolve ambiguities (this can't have any

This also works:

-%nonassoc  UNBOUNDED   /* ideally should have same
precedence as IDENT */
-%nonassoc  IDENT PARTITION RANGE ROWS GROUPS PRECEDING FOLLOWING
CUBE ROLLUP
+%nonassoc UNBOUNDED IDENT PARTITION RANGE ROWS GROUPS CUBE ROLLUP
+%nonassoc PRECEDING FOLLOWING

Not sure if either is better. Some additional input would be good here.

While looking for a place to put a v13 deprecation notice, I found
some more places in the docs which need updating:

ref/create_operator.sgml

"At least one of LEFTARG and RIGHTARG must be defined. For binary
operators, both must be defined. For right unary operators, only
LEFTARG should be defined, while for left unary operators only
RIGHTARG should be defined."

ref/create_opclass.sgml

"In an OPERATOR clause, the operand data type(s) of the operator, or
NONE to signify a left-unary or right-unary operator."

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

Re: Handing off SLRU fsyncs to the checkpointer

[Jakub Wartak]

Hi Thomas / hackers,

>> The append-only bottleneck appears to be limited by syscalls/s due to small 
>> block size even with everything in FS cache (but not in shared buffers, 
>> please compare with TEST1 as there was no such bottleneck at all):
>>
>> 29.62%  postgres  [kernel.kallsyms]   [k] copy_user_enhanced_fast_string
>> ---copy_user_enhanced_fast_string
>>|--17.98%--copyin
>> [..]
>>|  __pwrite_nocancel
>>|  FileWrite
>>|  mdwrite
>>|  FlushBuffer
>>|  ReadBuffer_common
>>|  ReadBufferWithoutRelcache
>>|  XLogReadBufferExtended
>>|  XLogReadBufferForRedoExtended
>>|   --17.57%--btree_xlog_insert
>
> To move these writes out of recovery's way, we should probably just
> run the bgwriter process during crash recovery.  I'm going to look
> into that.

Sounds awesome. Also as this thread is starting to derail the SLRU fsync topic 
- maybe we should change subject? However, to add some data to the separate 
bgwriter: on 14master (already with lseek() caching, SLRU fsyncs out of way, 
better sorting), I've measured the same configuration as last time with still 
the same append-only WAL workload on NVMe and compared with various 
shared_buffers settings (and buffers description sizing from 
pg_shmem_allocations which as You stated is wrongly reported(?) which I'm 
stating only for reference just in case):

shared_buffers=128MB buffers_desc=1024kB 96.778, 0.438 [a]
shared_buffers=256MB buffers_desc=2048kB 62.755, 0.577 [a]
shared_buffers=512MB buffers_desc=4096kB 33.167, 0.62 [a]
shared_buffers=1GB buffers_desc=8192kB 27.303, 0.929
shared_buffers=4GB buffers_desc=32MB 27.185, 1.166
shared_buffers=8GB buffers_desc=64MB 27.649, 1.088 
shared_buffers=16GB buffers_desc=128MB 27.584, 1.201 [b]
shared_buffers=32GB buffers_desc=256MB 32.314, 1.171 [b]
shared_buffers=48GB buffers_desc=384 MB 31.95, 1.217
shared_buffers=64GB buffers_desc=512 MB 31.276, 1.349
shared_buffers=72GB buffers_desc=576 MB 31.925, 1.284
shared_buffers=80GB buffers_desc=640 MB 31.809, 1.413

The amount of WAL to be replayed was ~2.8GB. To me it looks like that
a) flushing dirty buffers by StartupXLog might be a real problem but please 
read-on.
b) there is very low impact by this L2/L3 hypothesis you mention (?), but it's 
not that big and it's not degrading linearly as one would expect (??) 
L1d:L1d:L2:L3 cache sizes on this machine are as follows on this machine: 
32K/32K/256K/46080K. Maybe the DB size is too small.

I've double-checked that in condition [a] (shared_buffers=128MB) there was a 
lot of FlushBuffer() invocations per second (perf stat -e 
probe_postgres:FlushBuffer -I 1000), e.g:
#   time counts unit events
 1.000485217 79,494  probe_postgres:FlushBuffer
 2.000861366 75,305  probe_postgres:FlushBuffer
 3.001263163 79,662  probe_postgres:FlushBuffer
 4.001650795 80,317  probe_postgres:FlushBuffer
 5.002033273 79,951  probe_postgres:FlushBuffer
 6.002418646 79,993  probe_postgres:FlushBuffer
while at 1GB shared_buffers it sits nearly at zero all the time.  So there is 
like 3x speed-up possible  when StartupXLog wouldn't have to care too much 
about dirty buffers in the critical path (bgwriter would as you say?) at least 
in append-only scenarios.  But ... I've checked some real systems (even older 
versions of PostgreSQL not doing that much of replication, and indeed it's  
e.g. happening 8-12k/s FlushBuffer's() and shared buffers are pretty huge 
(>100GB, 0.5TB RAM) but they are *system-wide* numbers, work is really 
performed by bgwriter not by startup/recovering as in this redo-bench case when 
DB is open for reads and replicating-- so it appears that this isn't 
optimization for hot standby case , but for the DB-closed startup 
recovery/restart/disaster/PITR scenario).

As for the 24GB shared_buffers scenario where dirty buffers are not at all a 
problem with given top profile (output trimmed), again as expected:

13.41%  postgres  postgres   [.] hash_search_with_hash_value
   |--8.31%--BufTableLookup <- ReadBuffer_common <- 
ReadBufferWithoutRelcache
--5.11%--smgropen 
  |--2.77%--XLogReadBufferExtended
   --2.34%--ReadBufferWithoutRelcache
 7.88%  postgres  postgres   [.] MarkBufferDirty

I've tried to get cache misses ratio via PMCs, apparently on EC2 they are (even 
on bigger) reporting as not-supported or zeros. However interestingly the 
workload has IPC of 1.40 (instruction bound) which to me is strange as I would 
expect BufTableLookup() to be actually heavy memory bound (?) Maybe I'll try on 
some different hardware one day. 

>>   

Re: Help needed configuring postgreSQL with xml support

[Ashutosh Sharma]

In addition to what Thomas said, I would also recommend you to refer
to the description of --with-libxml command line option provided in
the postgres installation-procedure page - [1].

[1] - https://www.postgresql.org/docs/12/install-procedure.html

--
With Regards,
Ashutosh Sharma
EnterpriseDB:http://www.enterprisedb.com

On Thu, Aug 27, 2020 at 1:56 PM Thomas Munro  wrote:
>
> On Thu, Aug 27, 2020 at 8:17 PM Khanna, Sachin 000
>  wrote:
> > I am getting following error in  configuration.log of installation . Please 
> > help
>
> You didn't mention what operating system this is, but, for example, if
> it's Debian, Ubuntu or similar you might need to install libxml2-dev
> and pkg-config for --with-libxml to work.
>
>

Some two phase optimization ideas

[Paul Guo]

Hello hackers,

While working on two phase related issues, I found something related to two 
phase could be optimized.

1. The current implementation decouples PREPRE and COMMIT/ABORT PREPARE a lot. 
This is flexible, but if
PREPARE & COMMIT/ABORT mostly happens on the same backend we could use the 
cache mechanism to
speed up, e.g.

  a. FinishPreparedTransaction()->LockGXact(gid, user)
   for (i = 0; i < TwoPhaseState->numPrepXacts; i++)
find the gxact that matches gid 
   
  For this we can cache the gxact during PREPARE and use that for a 
fast path, i.e. if the cached gxact
  matches gid we do not need to walk through the gxact array. By the 
way, if the gxact array is large this
  will be a separate performance issue (use shared-memory hash table if 
needed?).

  b. FinishPreparedTransaction() reads the PREPARE information from either 
state file (stored during checkpoint)
  or wal file. We could cache the content during PREPARE, i.e. in 
EndPrepare() then in FinishPreparedTransaction()
 we can avoid reading the state file or the wal file.

   It is possible that some databases based on Postgres two phase might not 
want the cache, e.g. if PREPARE
   backend is always different than the COMMIT/ABORT PREPARE backend (I do 
not know what database is
  designing like this though), but gxact caching is almost no overhead and 
for b we could use ifdef to guard the
  PREPARE wal data copying code.

 The two optimizations are easy and small. I've verified on Greenplum 
database (based on Postgres 12).

2. wal content duplication between PREPARE and COMMT/ABORT PREPARE
 
 See the below COMMIT PREPARE function call. Those hdr->* have existed in 
PREPARE wal also. We do
 not need them in the COMMIT PREPARE wal also. During recovery, we could 
load these information (both
 COMMIT and ABORT) into memory and in COMMIT/ABORT PREPARE redo we use the 
corresponding data.

  RecordTransactionCommitPrepared(xid,
  hdr->nsubxacts, children,
  hdr->ncommitrels, commitrels,
  hdr->ninvalmsgs, invalmsgs,
  hdr->initfileinval, gid);

  One drawback of the change is this might involve non-trivial change.

3. About gid, current gid is defined as a char[]. I'm wondering if we should 
define an opaque type and let some
Databases implement their own gid types using callbacks. Typically if I 
want to use 64-bit distributed xid as gid,
current code is not that performance & storage friendly (e.g. still need to 
use strcmp to find gxact in LockGXact,).
We may implement a default implementation as char[]. gid is not widely used 
so the change seems to
be small (interfaces of copy, comparison, conversion from string to 
internal gid type for the PREPARE statement, etc)

Any thoughts?

Regards,
Paul

Support for OUT parameters in procedures

[Peter Eisentraut]

Procedures currently don't allow OUT parameters.  The reason for this is 
that at the time procedures were added (PG11), some of the details of 
how this should work were unclear and the issue was postponed.  I am now 
intending to resolve this.


AFAICT, OUT parameters in _functions_ are not allowed per the SQL 
standard, so whatever PostgreSQL is doing there at the moment is mostly 
our own invention.  By contrast, I am here intending to make OUT 
parameters in procedures work per SQL standard and be compatible with 
the likes of PL/SQL.


The main difference is that for procedures, OUT parameters are part of 
the signature and need to be specified as part of the call.  This makes 
sense for nested calls in PL/pgSQL like this:


CREATE PROCEDURE test_proc(IN a int, OUT b int)
LANGUAGE plpgsql
AS $$
BEGIN
  b := a * 2;
END;
$$;

DO $$
DECLARE _a int; _b int;
BEGIN
  _a := 10;
  CALL test_proc(_a, _b);
  RAISE NOTICE '_a: %, _b: %', _a, _b;
END
$$;

For a top-level direct call, you can pass whatever you want, since all 
OUT parameters are presented as initially NULL to the procedure code. 
So you could just pass NULL, as in CALL test_proc(5, NULL).


The code changes to make this happen are not as significant as I had 
initially feared.  Most of the patch is expanded documentation and 
additional tests.  In some cases, I changed the terminology from "input 
parameters" to "signature parameters" to make the difference clearer. 
Overall, while this introduces some additional conceptual complexity, 
the way it works is pretty obvious in the end, and people porting from 
other systems will find it working as expected.


--
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From 27aa494f2f538d8d267841d5bd524b0eb3c77adb Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Wed, 26 Aug 2020 23:11:55 +0200
Subject: [PATCH v1] Support for OUT parameters in procedures

Unlike for functions, OUT parameters for procedures are part of the
signature.  Therefore, they have to be listed in pg_proc.proargtypes
as well as mentioned in ALTER PROCEDURE and DROP PROCEDURE.
---
 doc/src/sgml/catalogs.sgml|  5 +-
 doc/src/sgml/plpgsql.sgml | 38 
 doc/src/sgml/ref/alter_extension.sgml | 11 ++-
 doc/src/sgml/ref/alter_procedure.sgml |  5 +-
 doc/src/sgml/ref/comment.sgml | 11 ++-
 doc/src/sgml/ref/create_procedure.sgml|  6 +-
 doc/src/sgml/ref/drop_procedure.sgml  |  5 +-
 doc/src/sgml/ref/security_label.sgml  | 11 ++-
 doc/src/sgml/xfunc.sgml   | 59 
 src/backend/commands/functioncmds.c   | 52 ++
 src/backend/parser/gram.y | 96 ---
 src/include/catalog/pg_proc.h |  2 +-
 src/pl/plperl/expected/plperl_call.out| 18 
 src/pl/plperl/sql/plperl_call.sql | 20 
 src/pl/plpgsql/src/expected/plpgsql_call.out  | 19 
 src/pl/plpgsql/src/pl_comp.c  |  1 +
 src/pl/plpgsql/src/sql/plpgsql_call.sql   | 21 
 src/pl/plpython/expected/plpython_call.out| 17 
 src/pl/plpython/plpy_procedure.c  |  4 +-
 src/pl/plpython/sql/plpython_call.sql | 19 
 src/pl/tcl/expected/pltcl_call.out| 17 
 src/pl/tcl/sql/pltcl_call.sql | 19 
 .../regress/expected/create_procedure.out | 16 +++-
 src/test/regress/sql/create_procedure.sql | 13 ++-
 24 files changed, 400 insertions(+), 85 deletions(-)

diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 9fe260ecff..a071154123 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -5865,8 +5865,9 @@ pg_proc Columns
   
An array with the data types of the function arguments.  This includes
only input arguments (including INOUT and
-   VARIADIC arguments), and thus represents
-   the call signature of the function.
+   VARIADIC arguments), as well as
+   OUT parameters of procedures, and thus represents
+   the call signature of the function or procedure.
   
  
 
diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml
index 815912666d..309a714fc4 100644
--- a/doc/src/sgml/plpgsql.sgml
+++ b/doc/src/sgml/plpgsql.sgml
@@ -478,6 +478,14 @@ Declaring Function Parameters
   included it, but it would be redundant.
  
 
+ 
+  To call a function with OUT parameters, omit the
+  output parameter in the function call:
+
+SELECT sales_tax(100.00);
+
+ 
+
  
   Output parameters are most useful when returning multiple values.
   A trivial example is:
@@ -489,6 +497,11 @@ Declaring Function Parameters
 prod := x * y;
 END;
 $$ LANGUAGE plpgsql;
+
+SELECT * FROM sum_n_product(2, 4);
+ sum | prod
+-+--
+   6 |8
 
 
   As discussed in , this
@@ -497,6 +510,31 @@ Declaring Functi

Re: Help needed configuring postgreSQL with xml support

[Thomas Munro]

On Thu, Aug 27, 2020 at 8:17 PM Khanna, Sachin 000
 wrote:
> I am getting following error in  configuration.log of installation . Please 
> help

You didn't mention what operating system this is, but, for example, if
it's Debian, Ubuntu or similar you might need to install libxml2-dev
and pkg-config for --with-libxml to work.

Help needed configuring postgreSQL with xml support

[Khanna, Sachin 000]

I am getting following error in  configuration.log of installation . Please help

Is there any pkg-config path that needs to be configured/set in environment for 
this to work.

configure:8172: checking for libxml-2.0 >= 2.6.23
configure:8179: $PKG_CONFIG --exists --print-errors "libxml-2.0 >= 2.6.23"
Package libxml-2.0 was not found in the pkg-config search path.
Perhaps you should add the directory containing `libxml-2.0.pc'
to the PKG_CONFIG_PATH environment variable
No package 'libxml-2.0' found
configure:8182: $? = 1
configure:8196: $PKG_CONFIG --exists --print-errors "libxml-2.0 >= 2.6.23"
Package libxml-2.0 was not found in the pkg-config search path.
Perhaps you should add the directory containing `libxml-2.0.pc'
to the PKG_CONFIG_PATH environment variable
No package 'libxml-2.0' found
configure:8199: $? = 1
configure:8213: result: no
No package 'libxml-2.0' found

I later tried setting the some environment variables like


export XML2_CFLAGS='/usr/lib64/'
export XML2_CONFIG='/usr/lib64/'
export XML2_LIBS='/usr/lib64/libxml2

Created soft link as well libxml2.so -> /usr/lib64/libxml2.so.2.9.1 but now I 
getting the below error .

configure: error: header file  is required for XML support

>From config.log

configure:13266: checking libxml/parser.h usability
configure:13266: gcc -std=gnu99 -c -Wall -Wmissing-prototypes -Wpointer-arith 
-Wdeclaration-after-statement -Werror=vla -Wendif-labels 
-Wmissing-format-attribute -Wformat-security -fno-strict-aliasing -fwrapv 
-fexcess-precision=standard -O2  -D_GNU_SOURCE  conftest.c >&5
conftest.c:96:27: fatal error: libxml/parser.h: No such file or directory
#include 
   ^
compilation terminated.
configure:13266: $? = 1
configure: failed program was:




Thanks and Regards,
SACHIN KHANNA
212 Basis Offshore DBA
Office : 204058624
Cell : 9049522511
Email: khanna.sac...@corp.sysco.com

ITIL V3 (F), AWS Certified Solution Archtect
Infosys Technologies Limited (r) | PUNE

Re: Add Information during standby recovery conflicts

[Masahiko Sawada]

On Mon, 10 Aug 2020 at 23:43, Drouvot, Bertrand  wrote:
>
> Hi,
>
> On 7/31/20 7:12 AM, Masahiko Sawada wrote:
> > +   tmpWaitlist = waitlist;
> > +   while (VirtualTransactionIdIsValid(*tmpWaitlist))
> > +   {
> > +   tmpWaitlist++;
> > +   }
> > +
> > +   num_waitlist_entries = (tmpWaitlist - waitlist);
> > +
> > +   /* display wal record information */
> > +   if (log_recovery_conflicts &&
> > (TimestampDifferenceExceeds(recovery_conflicts_log_time,
> > GetCurrentTimestamp(),
> > +   DeadlockTimeout))) {
> > +   LogBlockedWalRecordInfo(num_waitlist_entries, reason);
> > +   recovery_conflicts_log_time = GetCurrentTimestamp();
> > +   }
> >
> > recovery_conflicts_log_time is not initialized. And shouldn't we
> > compare the current timestamp to the timestamp when the startup
> > process started waiting?
> >
> > I think we should call LogBlockedWalRecordInfo() inside of the inner
> > while loop rather than at the beginning of
> > ResolveRecoveryConflictWithVirtualXIDs(). In lock conflict cases, the
> > startup process waits until 'ltime', then enters
> > ResolveRecoveryConflictWithVirtualXIDs() after reaching 'ltime'.
> > Therefore, it makes sense to call LogBlockedWalRecordInfo() at the
> > beginning of ResolveRecoveryConflictWithVirtualXIDs(). However, in
> > snapshot and tablespace conflict cases (i.g.
> > ResolveRecoveryConflictWithSnapshot() and
> > ResolveRecoveryConflictWithTablespace()), it enters
> > ResolveRecoveryConflictWithVirtualXIDs() without waits and waits for
> > reaching ‘ltime’ inside of the inner while look. So the above
> > condition could always be false.
>
> That would make the information being displayed after
> max_standby_streaming_delay is reached for the multiple cases you just
> described.

Sorry, it should be deadlock_timeout, not max_standby_streaming_delay.
Otherwise, the recovery conflict log message is printed when
resolution, which seems not to achieve the original purpose. Am I
missing something?

Regards,

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

Re: Typo in procarray.c comment about GlobalVisDataRels

[Michael Paquier]

On Wed, Aug 26, 2020 at 04:22:51PM -0500, Jim Nasby wrote:
> The comment in procarray.c that described GlobalVisDataRels instead
> mentioned GlobalVisCatalogRels a second time. Patch attached.

Thanks, Jim.  Applied.
--
Michael


signature.asc
Description: PGP signature

Re: "cert" + clientcert=verify-ca in pg_hba.conf?

[Kyotaro Horiguchi]

At Wed, 26 Aug 2020 18:36:50 -0400, Bruce Momjian  wrote in 
bruce> On Wed, Aug 26, 2020 at 06:13:23PM +0900, Kyotaro Horiguchi wrote:
> > At Tue, 25 Aug 2020 22:52:44 -0400, Bruce Momjian  wrote 
> > in 
> > > > Because we think we need any named value for every alternatives
> > > > including the default value?
> > > 
> > > Well, not putting clientcert at all gives the default behavior, so why
> > > have clientcert=no-verify?
> > 
> > clientcert=verify-ca or verify-full don't allow absence of client
> > certificate. We need an option to allow the absence.
> 
> Isn't the option not specifying clientcert?  Here are some valid
> pg_hba.conf lines:

Sorry for the ambiguity. Perhaps I understand that we talked at
different objects.  I was mentioning about the option value that is
stored *internally*, concretely the values for the struct member
port->hba->clientcert. You are talking about the descriptive option in
pg_hba.conf.

Does the following discussion make sense?

We need to use the default value zero (=clientCertOff) for
port->hba->clientcert to tell server to omit checking against CA if
cert is not given.  I suppose that the value clientCertOff is labeled
as "no-verify" since someone who developed this thought that that
choice needs to be explicitly describable in pg_hba.conf. And my
discussion was following that decision.

I understand that the label "no-verify" is not essential to specify
the behavior, so I don't object to removing "no-verify" label itself
if no one oppose to remove it.

My point here is just "are we OK to remove it?"

> It is my understanding that the last two lines are the same.  Why isn't
> it sufficient to just tell users not to specify clientcert if they want
> the default behavior?  You can do:
> 
>   hostall all 192.168.0.0/16  ident 
> map=omicron
> 
> but there is no way to specify the default map value of 'no map', so why
> have one for clientcert?

The difference from clientcert is that it gives an arbitrary name that
points to a defined mapping, not a choice from an defined
enumeration. 

> > > Well, sslmode=prefer gives encryption without identification.
> > > clientcert=no-verify has no value because it is just an optional CA
> > > check that has no value because optional authentication is useless.  It
> > 
> > The point of the option is not to do optional CA check if possible,
> > but to allow absence of client cert. We need to have that mode
> > regardless of named or not named, and I believe we usually provide a
> > name for default mode.
> 
> Uh, see above --- not really.  The absense of the option is the default
> action.


regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center