Re: ALTER tbl rewrite loses CLUSTER ON index

[Amit Langote]

On Fri, Mar 13, 2020 at 2:19 AM Tom Lane  wrote:
> Justin Pryzby  writes:
> > @cfbot: resending with only Amit's 0001, since Michael pushed a variation on
> > 0002.

Thank you for taking a look at it.

> Boy, I really dislike this patch.  ATPostAlterTypeParse is documented as
> using the supplied definition string, and nothing else, to reconstruct
> the index.  This breaks that without even the courtesy of documenting
> the breakage.  Moreover, the reason why it's designed like that is to
> avoid requiring the old index objects to still be accessible.  So I'm
> surprised that this hack works at all.  I don't think it would have
> worked at the time the code was first written, and I think it's imposing
> a constraint we'll have problems with (again?) in future.

Okay, so maybe in the middle of ATPostAlterTypeParse() is not a place
to do it, but don't these arguments apply to
RebuildConstraintComment(), which I based the patch on?

> The right way to fix this is to note the presence of the indisclustered
> flag when we're examining the index earlier, and include a suitable
> command in the definition string.  So probably pg_get_indexdef_string()
> is what needs to change.  It doesn't look like that's used anywhere
> else, so we can just redefine its behavior as needed.

I came across a commit that recently went in:

commit 1cc9c2412cc9a2fbe6a381170097d315fd40ccca
Author: Peter Eisentraut 
Date:   Fri Mar 13 11:28:11 2020 +0100

Preserve replica identity index across ALTER TABLE rewrite

which fixes something very similar to what we are trying to with this
patch.  The way it's done looks to me very close to what you are
telling.  I have updated the patch to be similar to the above fix.

--
Thank you,
Amit


v6-0001-ALTER-tbl-rewrite-loses-CLUSTER-ON-index.patch
Description: Binary data

Re: Internal key management system

[Masahiko Sawada]

On Thu, 12 Mar 2020 at 08:13, Bruce Momjian
 wrote:
>
> On Fri, Mar  6, 2020 at 03:31:00PM +0900, Masahiko Sawada wrote:
> > On Fri, 6 Mar 2020 at 15:25, Moon, Insung  
> > wrote:
> > >
> > > Dear Sawada-san
> > >
> > > I don't know if my environment or email system is weird, but the V5
> > > patch file is only include simply a changed list.
> > > and previous V4 patch file size was 64kb, but the v5 patch file size was 
> > > 2kb.
> > > Can you check it?
> > >
> >
> > Thank you! I'd attached wrong file.
>
> Looking at this thread, I wanted to make a few comments:
>
> Everyone seems to think pgcrypto need some maintenance.  Who would like
> to take on that task?
>
> This feature does require openssl since all the encryption/decryption
> happen via openssl function calls.
>
> Three are three levels of encrypt here:
>
> 1.  The master key generated during initdb
>
> 2.  The passphrase to unlock the master key at boot time.  Is that
> optional or required?

The passphrase is required if the internal kms is enabled during
initdb. Currently hashing the passphrase is also required but it could
be optional. Even if we make hashing optional, we still require
openssl to wrap and unwrap.

>
> 3.  The wrap/unwrap key, which can be per-user/application/cluster
>
> In the patch, the doc heading "Cluster Encryption Key Rotation" seems
> confusing.  Doesn't that change the pass phrase?  Why refer to it as the
> cluster encryption key here?

Agreed, changed to "Changing Cluster Passphrase".

>
> Could the wrap functions expose the master encryption key by passing in
> empty string or null?

Currently the wrap function returns NULL if null is passed, and
doesn't expose the master encryption key even if empty string is
passed because we add random IV for each wrapping.

>  I wonder if we should create a derived key from
> the master key to use for pg_wrap/pg_unwrap, maybe by appending a fixed
> string to all strings supplied to these functions.  We could create
> another derived key for use in block-level encryption, so we are sure
> the two key spaces would never overlap.

Currently the master key is 32 bytes but you mean to add fixed string
to the master key to derive a new key?

>
> pgcryptokey shows a method for creating a secret between client and
> server using SQL that does not expose the secret in the server logs:
>
> https://momjian.us/download/pgcryptokey/
>
> I assume we will create a 256-bit key for the master key, but give users
> an option of  128-bit vs 256-bit keys for block-level encryption.
> 256-bit keys are considered necessary for security against future
> quantum computing attacks.

256-bit keys are more weaker than 128-bit key in terms of quantum
computing attacks?

>
> This looks like a bug in the patch:
>
> -This parameter can only be set in the 
> postgresql.conf
> +This parameter can only be set in the 
> postgresql.confo

Fixed.

Regards,

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

Re: add types to index storage params on doc

[Atsushi Torikoshi]

Thanks for your comments!

On Mon, Mar 16, 2020 at 11:49 AM Fujii Masao 
wrote:

> -buffering
> +buffering (string)
>
> Isn't it better to use "enum" rather than "string"?
> In the docs about enum GUC parameters, "enum" is used there.
>

Agreed. I've fixed it to "enum".

But I'm now wondering about the type of check_option[3], [4].
Because I decide the type to "string" referring to check_option, which is
the other element of  enumRelOpts in reloptions.c.

Should I also change it to "enum"?

[3]https://www.postgresql.org/docs/devel/sql-alterview.html#id-1.9.3.45.6
[4]https://www.postgresql.org/docs/devel/sql-createview.html#id-1.9.3.97.6

Regards,
--
Torikoshi Atsushi


add_type_in_createindex_storage-params_v2.patch
Description: Binary data

Re: Berserk Autovacuum (let's save next Mandrill)

[Laurenz Albe]

On Mon, 2020-03-16 at 12:53 +0900, Masahiko Sawada wrote:
> There is already a consensus on introducing new 2 parameters, but as
> the second idea I'd like to add one (or two) GUC(s) to my suggestion,
> say autovacuum_vacuum_freeze_insert_ratio; this parameter is the ratio
> of the number of inserted tuples for total number of tuples modified
> and inserted, in order to trigger insert-only vacuum. For example,
> suppose the table has 1,000,000 tuples and we set threshold = 0,
> scale_factor = 0.2 and freeze_insert_ratio = 0.9, we will trigger
> normal autovacuum when n_dead_tup + n_ins_since_vacuum > 200,000, but
> we will instead trigger insert-only autovacuum, which is a vacuum with
> vacuum_freeze_min_age = 0, when n_ins_since_vacuum > 180,000 (=200,000
> * 0.9). IOW if 90% of modified tuples are insertions, we freeze tuples
> aggressively. If we want to trigger insert-only vacuum only on
> insert-only table we can set freeze_insert_ratio = 1.0. The down side
> of this idea is that we cannot disable autovacuum triggered by the
> number of inserted, although we might be able to introduce more one
> GUC that controls whether to include the number of inserted tuples for
> triggering autovacuum (say, autovacuum_vacuum_triggered_by_insert =
> on|off). The pros of this idea would be that we can ensure that
> insert-only vacuum will run only in the case where the ratio of
> insertion is large enough.

Two more parameters :^(  But your reasoning is good.

How about we go with what we have now and leave that for future
discussion and patches?

Yours,
Laurenz Albe

Re: Collation versioning

[Michael Paquier]

On Thu, Mar 12, 2020 at 03:00:26PM +0100, Julien Rouhaud wrote:
> And v15 due to conflict with b08dee24a5 (Add pg_dump support for ALTER obj
> DEPENDS ON EXTENSION).

I have looked at patches 0001~0003 in the set for now.  0001 looks
clean to me.

In patch 0002, you have the following addition:
@@ -103,9 +103,10 @@ ORDER BY 1, 2;
  pg_class| relacl| aclitem[]
  pg_class| reloptions| text[]
  pg_class| relpartbound  | pg_node_tree
+ pg_depend   | refobjversion | text
This comes from a regression test doing a sanity check to look for
catalogs which have a toastable column but no toast tables.  As an
exception, it should be documented in the test's comment.  Actually,
does it need to be an exception?  This does not depend on
relation-level facilities so there should be no risk of recursive
dependencies, though I have not looked in details at this part.

+  
+   The only current use of refobjversion is to
+   record dependencies between indexes and collation versions.
+  
[...]
+ 
+  refobjversion
+  text
+  
+  
+   An optional version for the referenced object; see text
+  
+ 
Couldn't you merge both paragraphs here?

Regarding patch 0003, it would be nice to include some tests
independent on the rest and making use of the new functions.  These
normally go in regproc.sql.  For example with a collation that needs
double quotes as this is not obvious:
=# select regcollation('"POSIX"');
regcollation
 --
 "POSIX"
(1 row)

On top of that, this needs tests with to_regcollation() and tests with
schema-qualified collations.

Documentation for to_regcollation() is missing.  And it looks that
many parts of the documentation are missing an update.  One example in
datatype.sgml:
Type oid represents an object identifier.  There are also
several alias types for oid: regproc,
regprocedure, regoper, regoperator,
regclass, regtype, regrole,
regnamespace, regconfig, and
regdictionary.   shows an
overview.
At quick glance, there are more sections in need of a refresh..
--
Michael


signature.asc
Description: PGP signature

Re: [Proposal] Global temporary tables

[Konstantin Knizhnik]

On 16.03.2020 9:23, Prabhat Sahu wrote:

Hi Wenjing,
Please check the below scenario, where the Foreign table on GTT not 
showing records.


postgres=# create extension postgres_fdw;
CREATE EXTENSION
postgres=# do $d$
    begin
        execute $$create server fdw foreign data wrapper postgres_fdw 
options (host 'localhost',dbname 'postgres',port 
'$$||current_setting('port')||$$')$$;

    end;
$d$;
DO
postgres=# create user mapping for public server fdw;
CREATE USER MAPPING

postgres=# create table lt1 (c1 integer, c2 varchar(50));
CREATE TABLE
postgres=# insert into lt1 values (1,'c21');
INSERT 0 1
postgres=# create foreign table ft1 (c1 integer, c2 varchar(50)) 
server fdw options (table_name 'lt1');

CREATE FOREIGN TABLE
postgres=# select * from ft1;
 c1 | c2
+-
  1 | c21
(1 row)

postgres=# create global temporary table gtt1 (c1 integer, c2 
varchar(50));

CREATE TABLE
postgres=# insert into gtt1 values (1,'gtt_c21');
INSERT 0 1
postgres=# create foreign table f_gtt1 (c1 integer, c2 varchar(50)) 
server fdw options (table_name 'gtt1');

CREATE FOREIGN TABLE

postgres=# select * from gtt1;
 c1 |   c2
+-
  1 | gtt_c21
(1 row)

postgres=# select * from f_gtt1;
 c1 | c2
+
(0 rows)

--

With Regards,
Prabhat Kumar Sahu
EnterpriseDB: http://www.enterprisedb.com 



It seems to be expected behavior: GTT data is private to the session and 
postgres_fdw establish its own session where content of the table is empty.
But if you insert some data in f_gtt1, then you will be able to select 
this data from it because of connection cache in postgres_fdw.


--

Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: [Proposal] Global temporary tables

[曾文旌(义从)]

> 2020年3月16日 下午2:23，Prabhat Sahu  写道：
> 
> Hi Wenjing,
> Please check the below scenario, where the Foreign table on GTT not showing 
> records.
> 
> postgres=# create extension postgres_fdw;
> CREATE EXTENSION
> postgres=# do $d$
> begin
> execute $$create server fdw foreign data wrapper postgres_fdw options 
> (host 'localhost',dbname 'postgres',port '$$||current_setting('port')||$$')$$;
> end;
> $d$;
> DO
> postgres=# create user mapping for public server fdw;
> CREATE USER MAPPING
> 
> postgres=# create table lt1 (c1 integer, c2 varchar(50));
> CREATE TABLE
> postgres=# insert into lt1 values (1,'c21');
> INSERT 0 1
> postgres=# create foreign table ft1 (c1 integer, c2 varchar(50)) server fdw 
> options (table_name 'lt1');
> CREATE FOREIGN TABLE
> postgres=# select * from ft1;
>  c1 | c2  
> +-
>   1 | c21
> (1 row)
> 
> postgres=# create global temporary table gtt1 (c1 integer, c2 varchar(50));
> CREATE TABLE
> postgres=# insert into gtt1 values (1,'gtt_c21');
> INSERT 0 1
> postgres=# create foreign table f_gtt1 (c1 integer, c2 varchar(50)) server 
> fdw options (table_name 'gtt1');
> CREATE FOREIGN TABLE
> 
> postgres=# select * from gtt1;
>  c1 |   c2
> +-
>   1 | gtt_c21
> (1 row)
> 
> postgres=# select * from f_gtt1;
>  c1 | c2 
> +
> (0 rows)
> 
> -- 

I understand that postgre_fdw works similar to dblink.
postgre_fdw access to the table requires a new connection.
The data in the GTT table is empty in the newly established connection.
Because GTT shares structure but not data between connections.

Try local temp table:
create temporary table ltt1 (c1 integer, c2 varchar(50));

insert into ltt1 values (1,'gtt_c21');

create foreign table f_ltt1 (c1 integer, c2 varchar(50)) server fdw options 
(table_name 'ltt1');

select * from ltt1;
 c1 |   c2
+-
  1 | gtt_c21
(1 row)

select * from l_gtt1;
ERROR:  relation "l_gtt1" does not exist
LINE 1: select * from l_gtt1;


Wenjing


> With Regards,
> Prabhat Kumar Sahu
> EnterpriseDB: http://www.enterprisedb.com 

Re: Online checksums verification in the backend

[Julien Rouhaud]

Thanks for the review Michael!

On Mon, Mar 16, 2020 at 12:29:28PM +0900, Michael Paquier wrote:
> On Wed, Mar 11, 2020 at 08:18:23AM +0100, Julien Rouhaud wrote:
> > The cfbot reported a build failure, so here's a rebased v2 which also 
> > contains
> > the pg_stat_report_failure() call and extra log info.
>
> + * - if a block is not found in shared_buffers, the LWLock is relased and the
> + *   block is read from disk without taking any lock.  If an error is 
> detected,
> + *   the read block will be discarded and retrieved again while holding the
> + *   LWLock.  This is because an error due to concurrent write is possible 
> but
> + *   very unlikely, so it's better to have an optimistic approach to limit
> + *   locking overhead
> This can be risky with false positives, no?


Do you mean high probability of false positive in the 1st iteration, so running
frequently the recheck that can't have false positive, not that the 2nd check
can lead to false positive?


> With a large amount of
> shared buffer eviction you actually increase the risk of torn page
> reads.  Instead of a logic relying on partition mapping locks, which
> could be unwise on performance grounds, did you consider different
> approaches?  For example a kind of pre-emptive lock on the page in
> storage to prevent any shared buffer operation to happen while the
> block is read from storage, that would act like a barrier.


Even with a workload having a large shared_buffers eviction pattern, I don't
think that there's a high probability of hitting a torn page.  Unless I'm
mistaken it can only happen if all those steps happen concurrently to doing the
block read just after releasing the LWLock:

- postgres read the same block in shared_buffers (including all the locking)
- dirties it
- writes part of the page

It's certainly possible, but it seems so unlikely that the optimistic lock-less
approach seems like a very good tradeoff.


>
> + * Vacuum's GUCs are used to avoid consuming too much resources while running
> + * this tool.
> Shouldn't this involve separate GUCs instead of the VACUUM ones?


We could but the access pattern looked so similar that it looked like a good
idea to avoid adding 2 new GUC for that to keep configuration simple.  Unless
there are objections I'll add them in the next version.

> I guess that this leads to the fact that this function may be better as
> a contrib module, with the addition of some better-suited APIs in core
> (see paragraph above).


Below?


>
> +Run
> +make check
> +or
> +make installcheck
> Why is installcheck mentioned here?


Oups, copy/pasto error from the original contrib module this stuff was
initially implemented as, will fix.

>
> I don't think that it is appropriate to place the SQL-callable part in
> the existing checksum.c.  I would suggest instead a new file, say
> checksumfuncs.c in src/backend/utils/adt/, holding any SQL functions
> for checksums.


Agreed.

>
> -SUBDIRS = perl regress isolation modules authentication recovery
>  subscription
> +SUBDIRS = perl regress isolation modules authentication check_relation \
> + recovery subscription
> It seems to me that this test would be a good fit for
> src/test/modules/test_misc/.


WFM.

>
> +static void
> +check_all_relations(TupleDesc tupdesc, Tuplestorestate *tupstore,
> + ForkNumber forknum)
> Per the argument of bloat, I think that I would remove
> check_all_relation() as this function could take a very long time to
> run, and just make the SQL function strict.


No objection.

>
> + * - if a block is dirty in shared_buffers, it's ignored as it'll be flushed 
> to
> + *   disk either before the end of the next checkpoint or during recovery in
> + *   case of unsafe shutdown
> Wouldn't it actually be a good thing to check that the page on storage
> is fine in this case?  This depends on the system settings and the
> checkpoint frequency, but checkpoint_timeout can be extended up to 1
> day.  And plenty of things could happen to the storage in one day,
> including a base backup that includes a corrupted page on storage,
> that this function would not be able to detect.


How could that lead to data corruption?  If postgres crashes before the
checkpoint completion, the block will be overwritten during recovery, and if a
base backup is taken the block will also be overwritten while replaying all the
required WALs.  Detecting a corrupted blocks in those cases would have the
merit of possibly warning about possibly broken hardware sooner, but it would
also make the check more expensive as the odds to prevent postgres from
evicting a dirty block is way higher.  Maybe an additional GUC for that?

For the record when I first tested that feature I did try to check dirty
blocks, and it seemed that dirty blocks of shared relation were sometimes
wrongly reported as corrupted.  I didn't try to investigate more though.


> + *   we detect if a block is in shared_buffers or not.  See get_buffer()
> + *   com

Re: Online checksums verification in the backend

[Julien Rouhaud]

On Mon, Mar 16, 2020 at 01:53:35PM +0900, Masahiko Sawada wrote:
>
> In addition to comments from Michael-san, here are my comments:
>
> 1.
> +   if (!is_member_of_role(GetUserId(), DEFAULT_ROLE_READ_SERVER_FILES))
> +   ereport(ERROR,
> +   (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
> +errmsg("only superuser or a member of the
> pg_read_server_files role may use this function")));


Good point!  I'll fix it.


> +
> +   if (!DataChecksumsEnabled())
> +   elog(ERROR, "Data checksums are not enabled");
>
> I think it's better to reverse the order of the above checks.


Indeed.


>
> 2.
> +#define CRF_COLS   5   /* Number of output arguments in the SRF */
>
> Should it be SRF_COLS?


Oops, will fix.


>
> 3.
> +static void
> +check_delay_point(void)
> +{
> +   /* Always check for interrupts */
> +   CHECK_FOR_INTERRUPTS();
> +
> +   /* Nap if appropriate */
> +   if (!InterruptPending && VacuumCostBalance >= VacuumCostLimit)
> +   {
> +   int msec;
> +
> +   msec = VacuumCostDelay * VacuumCostBalance / VacuumCostLimit;
> +   if (msec > VacuumCostDelay * 4)
> +   msec = VacuumCostDelay * 4;
> +
> +   pg_usleep(msec * 1000L);
> +
> +   VacuumCostBalance = 0;
> +
> +   /* Might have gotten an interrupt while sleeping */
> +   CHECK_FOR_INTERRUPTS();
> +   }
> +}
>
> Even if we use vacuum delay for this function, I think we need to set
> VacuumDelayActive and return if it's false, or it's better to just
> return if VacuumCostDelay == 0.


Good point, I'll fix that.


>
> 4.
> +static void
> +check_all_relations(TupleDesc tupdesc, Tuplestorestate *tupstore,
> + ForkNumber forknum)
>
> I also agree with Michael-san to remove this function. Instead we can
> check all relations by:
>
> select pg_check_relation(oid) from pg_class;


Sure, but ideally we should do that in a client program (eg.  pg_checksums)
that wouldn't maintain a transaction active for the whole execution.


> 6.
> Other typos
>
> s/dirted/dirtied/
> s/explictly/explicitly/


Will fix, thanks!

Re: [HACKERS] [PATCH] Generic type subscripting

[Pavel Stehule]

st 4. 3. 2020 v 18:04 odesílatel Dmitry Dolgov <9erthali...@gmail.com>
napsal:

> > On Tue, Mar 03, 2020 at 12:55:38PM -0500, David Steele wrote:
> >
> > > Yep, I wasn't paying much attention recently to this patch, will post
> > > rebased and fixed version soon.
> >
> > The last CF for PG13 has begun.  Do you know when you'll have a rebased
> and
> > updated patch available?  The current patch no longer applies.
>
> Thanks for the reminder! Here is the rebased version, with one extra
> detail. Looks like tests for non-strict version of jsonb_set are doing:
>
> \pset null NULL
>
> and backwards, but via:
>
> \pset null
>
> which is not changing it back. It's showed up in tests for subscripting
> down the file, so I've changed it to what I guess it was suppose to be:
>
> \pset null ''
>

Hi

please rebase this patch

Regards

Pavel

Re: WIP/PoC for parallel backup

[Jeevan Chalke]

Hi Asif,


> Thanks Rajkumar. I have fixed the above issues and have rebased the patch
> to the latest master (b7f64c64).
> (V9 of the patches are attached).
>

I had a further review of the patches and here are my few observations:

1.
+/*
+ * stop_backup() - ends an online backup
+ *
+ * The function is called at the end of an online backup. It sends out
pg_control
+ * file, optionally WAL segments and ending WAL location.
+ */

Comments seem out-dated.

2. With parallel jobs, maxrate is now not supported. Since we are now asking
data in multiple threads throttling seems important here. Can you please
explain why have you disabled that?

3. As we are always fetching a single file and as Robert suggested, let
rename
SEND_FILES to SEND_FILE instead.

4. Does this work on Windows? I mean does pthread_create() work on Windows?
I asked this as I see that pgbench has its own implementation for
pthread_create() for WIN32 but this patch doesn't.

5. Typos:
tablspace => tablespace
safly => safely

6. parallel_backup_run() needs some comments explaining the states it goes
through PB_* states.

7.
+case PB_FETCH_REL_FILES:/* fetch files from server */
+if (backupinfo->activeworkers == 0)
+{
+backupinfo->backupstate = PB_STOP_BACKUP;
+free_filelist(backupinfo);
+}
+break;
+case PB_FETCH_WAL_FILES:/* fetch WAL files from server */
+if (backupinfo->activeworkers == 0)
+{
+backupinfo->backupstate = PB_BACKUP_COMPLETE;
+}
+break;

Why free_filelist() is not called in PB_FETCH_WAL_FILES case?

Thanks
-- 
Jeevan Chalke
Associate Database Architect & Team Lead, Product Development
EnterpriseDB Corporation
The Enterprise PostgreSQL Company

Phone: +91 20 66449694

Website: www.enterprisedb.com
EnterpriseDB Blog: http://blogs.enterprisedb.com/
Follow us on Twitter: http://www.twitter.com/enterprisedb

This e-mail message (and any attachment) is intended for the use of the
individual or entity to whom it is addressed. This message contains
information from EnterpriseDB Corporation that may be privileged,
confidential, or exempt from disclosure under applicable law. If you are
not the intended recipient or authorized to receive this for the intended
recipient, any use, dissemination, distribution, retention, archiving, or
copying of this communication is strictly prohibited. If you have received
this e-mail in error, please notify the sender immediately by reply e-mail
and delete this message.

Re: [Proposal] Global temporary tables

[tushar]

Hi Wenjing,

I have created a global table on X session but i am not able to drop 
from Y session ?


X session - ( connect to psql terminal )
postgres=# create global temp table foo(n int);
CREATE TABLE
postgres=# select * from foo;
 n
---
(0 rows)


Y session - ( connect to psql terminal )
postgres=# drop table foo;
ERROR:  can not drop relation foo when other backend attached this 
global temp table


Table has been created  so i think - user should be able to drop from 
another session as well without exit from X session.


regards,

On 3/16/20 1:35 PM, 曾文旌(义从) wrote:



2020年3月16日 下午2:23，Prabhat Sahu > 写道：


Hi Wenjing,
Please check the below scenario, where the Foreign table on GTT not 
showing records.


postgres=# create extension postgres_fdw;
CREATE EXTENSION
postgres=# do $d$
    begin
        execute $$create server fdw foreign data wrapper postgres_fdw 
options (host 'localhost',dbname 'postgres',port 
'$$||current_setting('port')||$$')$$;

    end;
$d$;
DO
postgres=# create user mapping for public server fdw;
CREATE USER MAPPING

postgres=# create table lt1 (c1 integer, c2 varchar(50));
CREATE TABLE
postgres=# insert into lt1 values (1,'c21');
INSERT 0 1
postgres=# create foreign table ft1 (c1 integer, c2 varchar(50)) 
server fdw options (table_name 'lt1');

CREATE FOREIGN TABLE
postgres=# select * from ft1;
 c1 | c2
+-
  1 | c21
(1 row)

postgres=# create global temporary table gtt1 (c1 integer, c2 
varchar(50));

CREATE TABLE
postgres=# insert into gtt1 values (1,'gtt_c21');
INSERT 0 1
postgres=# create foreign table f_gtt1 (c1 integer, c2 varchar(50)) 
server fdw options (table_name 'gtt1');

CREATE FOREIGN TABLE

postgres=# select * from gtt1;
 c1 |   c2
+-
  1 | gtt_c21
(1 row)

postgres=# select * from f_gtt1;
 c1 | c2
+
(0 rows)

--


I understand that postgre_fdw works similar to dblink.
postgre_fdw access to the table requires a new connection.
The data in the GTT table is empty in the newly established connection.
Because GTT shares structure but not data between connections.

Try local temp table:
create temporary table ltt1 (c1 integer, c2 varchar(50));

insert into ltt1 values (1,'gtt_c21');

create foreign table f_ltt1 (c1 integer, c2 varchar(50)) server fdw 
options (table_name 'ltt1');


select * from ltt1;
 c1 |   c2
+-
  1 | gtt_c21
(1 row)

select * from l_gtt1;
ERROR:  relation "l_gtt1" does not exist
LINE 1: select * from l_gtt1;


Wenjing



With Regards,
Prabhat Kumar Sahu
EnterpriseDB: http://www.enterprisedb.com 





--
regards,tushar
EnterpriseDB  https://www.enterprisedb.com/
The Enterprise PostgreSQL Company

Re: [Proposal] Global temporary tables

[Pavel Stehule]

po 16. 3. 2020 v 9:58 odesílatel tushar 
napsal:

> Hi Wenjing,
>
> I have created a global table on X session but i am not able to drop from
> Y session ?
>
> X session - ( connect to psql terminal )
> postgres=# create global temp table foo(n int);
> CREATE TABLE
> postgres=# select * from foo;
>  n
> ---
> (0 rows)
>
>
> Y session - ( connect to psql terminal )
> postgres=# drop table foo;
> ERROR:  can not drop relation foo when other backend attached this global
> temp table
>
> Table has been created  so i think - user should be able to drop from
> another session as well without exit from X session.
>

By the original design GTT was not modifiable until is used by any session.
Now, you cannot to drop normal table when this table is used.

It is hard to say what is most correct behave and design, but for this
moment, I think so protecting table against drop while it is used by other
session is the best behave.

Maybe for next release we can introduce DROP TABLE x (FORCE) - like we have
for DROP DATABASE. This behave is very similar.

Pavel


> regards,
>
> On 3/16/20 1:35 PM, 曾文旌(义从) wrote:
>
>
>
> 2020年3月16日 下午2:23，Prabhat Sahu  写道：
>
> Hi Wenjing,
> Please check the below scenario, where the Foreign table on GTT not
> showing records.
>
> postgres=# create extension postgres_fdw;
> CREATE EXTENSION
> postgres=# do $d$
> begin
> execute $$create server fdw foreign data wrapper postgres_fdw
> options (host 'localhost',dbname 'postgres',port
> '$$||current_setting('port')||$$')$$;
> end;
> $d$;
> DO
> postgres=# create user mapping for public server fdw;
> CREATE USER MAPPING
>
> postgres=# create table lt1 (c1 integer, c2 varchar(50));
> CREATE TABLE
> postgres=# insert into lt1 values (1,'c21');
> INSERT 0 1
> postgres=# create foreign table ft1 (c1 integer, c2 varchar(50)) server
> fdw options (table_name 'lt1');
> CREATE FOREIGN TABLE
> postgres=# select * from ft1;
>  c1 | c2
> +-
>   1 | c21
> (1 row)
>
> postgres=# create global temporary table gtt1 (c1 integer, c2 varchar(50));
> CREATE TABLE
> postgres=# insert into gtt1 values (1,'gtt_c21');
> INSERT 0 1
> postgres=# create foreign table f_gtt1 (c1 integer, c2 varchar(50)) server
> fdw options (table_name 'gtt1');
> CREATE FOREIGN TABLE
>
> postgres=# select * from gtt1;
>  c1 |   c2
> +-
>   1 | gtt_c21
> (1 row)
>
> postgres=# select * from f_gtt1;
>  c1 | c2
> +
> (0 rows)
>
> --
>
>
> I understand that postgre_fdw works similar to dblink.
> postgre_fdw access to the table requires a new connection.
> The data in the GTT table is empty in the newly established connection.
> Because GTT shares structure but not data between connections.
>
> Try local temp table:
> create temporary table ltt1 (c1 integer, c2 varchar(50));
>
> insert into ltt1 values (1,'gtt_c21');
>
> create foreign table f_ltt1 (c1 integer, c2 varchar(50)) server fdw
> options (table_name 'ltt1');
>
> select * from ltt1;
>  c1 |   c2
> +-
>   1 | gtt_c21
> (1 row)
>
> select * from l_gtt1;
> ERROR:  relation "l_gtt1" does not exist
> LINE 1: select * from l_gtt1;
>
>
> Wenjing
>
>
> With Regards,
> Prabhat Kumar Sahu
> EnterpriseDB: http://www.enterprisedb.com
>
>
>
> --
> regards,tushar
> EnterpriseDB  https://www.enterprisedb.com/
> The Enterprise PostgreSQL Company
>
>

Re: [Proposal] Global temporary tables

[曾文旌(义从)]

> 2020年3月16日 下午4:58，tushar  写道：
> 
> Hi Wenjing,
> 
> I have created a global table on X session but i am not able to drop from Y 
> session ?
> 
> X session - ( connect to psql terminal )
> postgres=# create global temp table foo(n int);
> CREATE TABLE
> postgres=# select * from foo;
>  n 
> ---
> (0 rows)
> 
> 
> Y session - ( connect to psql terminal )
> postgres=# drop table foo;
> ERROR:  can not drop relation foo when other backend attached this global 
> temp table
For now, If one dba wants to drop one GTT,
he can use the view pg_gtt_attached_pids to see which backends are using this 
GTT.
then kill these sessions with pg_terminate_backend, and he can drop this GTT.

> 
> Table has been created  so i think - user should be able to drop from another 
> session as well without exit from X session. 
> 
> regards,
> 
> On 3/16/20 1:35 PM, 曾文旌(义从) wrote:
>> 
>> 
>>> 2020年3月16日 下午2:23，Prabhat Sahu >> > 写道：
>>> 
>>> Hi Wenjing,
>>> Please check the below scenario, where the Foreign table on GTT not showing 
>>> records.
>>> 
>>> postgres=# create extension postgres_fdw;
>>> CREATE EXTENSION
>>> postgres=# do $d$
>>> begin
>>> execute $$create server fdw foreign data wrapper postgres_fdw 
>>> options (host 'localhost',dbname 'postgres',port 
>>> '$$||current_setting('port')||$$')$$;
>>> end;
>>> $d$;
>>> DO
>>> postgres=# create user mapping for public server fdw;
>>> CREATE USER MAPPING
>>> 
>>> postgres=# create table lt1 (c1 integer, c2 varchar(50));
>>> CREATE TABLE
>>> postgres=# insert into lt1 values (1,'c21');
>>> INSERT 0 1
>>> postgres=# create foreign table ft1 (c1 integer, c2 varchar(50)) server fdw 
>>> options (table_name 'lt1');
>>> CREATE FOREIGN TABLE
>>> postgres=# select * from ft1;
>>>  c1 | c2  
>>> +-
>>>   1 | c21
>>> (1 row)
>>> 
>>> postgres=# create global temporary table gtt1 (c1 integer, c2 varchar(50));
>>> CREATE TABLE
>>> postgres=# insert into gtt1 values (1,'gtt_c21');
>>> INSERT 0 1
>>> postgres=# create foreign table f_gtt1 (c1 integer, c2 varchar(50)) server 
>>> fdw options (table_name 'gtt1');
>>> CREATE FOREIGN TABLE
>>> 
>>> postgres=# select * from gtt1;
>>>  c1 |   c2
>>> +-
>>>   1 | gtt_c21
>>> (1 row)
>>> 
>>> postgres=# select * from f_gtt1;
>>>  c1 | c2 
>>> +
>>> (0 rows)
>>> 
>>> -- 
>> 
>> I understand that postgre_fdw works similar to dblink.
>> postgre_fdw access to the table requires a new connection.
>> The data in the GTT table is empty in the newly established connection.
>> Because GTT shares structure but not data between connections.
>> 
>> Try local temp table:
>> create temporary table ltt1 (c1 integer, c2 varchar(50));
>> 
>> insert into ltt1 values (1,'gtt_c21');
>> 
>> create foreign table f_ltt1 (c1 integer, c2 varchar(50)) server fdw options 
>> (table_name 'ltt1');
>> 
>> select * from ltt1;
>>  c1 |   c2
>> +-
>>   1 | gtt_c21
>> (1 row)
>> 
>> select * from l_gtt1;
>> ERROR:  relation "l_gtt1" does not exist
>> LINE 1: select * from l_gtt1;
>> 
>> 
>> Wenjing
>> 
>> 
>>> With Regards,
>>> Prabhat Kumar Sahu
>>> EnterpriseDB: http://www.enterprisedb.com 
>> 
> 
> -- 
> regards,tushar
> EnterpriseDB  https://www.enterprisedb.com/ 
> The Enterprise PostgreSQL Company

Re: [Proposal] Global temporary tables

[曾文旌(义从)]

> 2020年3月16日 下午5:04，Pavel Stehule  写道：
> 
> 
> 
> po 16. 3. 2020 v 9:58 odesílatel tushar  > napsal:
> Hi Wenjing,
> 
> I have created a global table on X session but i am not able to drop from Y 
> session ?
> 
> X session - ( connect to psql terminal )
> postgres=# create global temp table foo(n int);
> CREATE TABLE
> postgres=# select * from foo;
>  n 
> ---
> (0 rows)
> 
> 
> Y session - ( connect to psql terminal )
> postgres=# drop table foo;
> ERROR:  can not drop relation foo when other backend attached this global 
> temp table
> 
> Table has been created  so i think - user should be able to drop from another 
> session as well without exit from X session. 
> 
> By the original design GTT was not modifiable until is used by any session. 
> Now, you cannot to drop normal table when this table is used.
> 
> It is hard to say what is most correct behave and design, but for this 
> moment, I think so protecting table against drop while it is used by other 
> session is the best behave.
> 
> Maybe for next release we can introduce DROP TABLE x (FORCE) - like we have 
> for DROP DATABASE. This behave is very similar.
I agree with that.


Wenjing

> 
> Pavel
> 
> 
> regards,
> 
> On 3/16/20 1:35 PM, 曾文旌(义从) wrote:
>> 
>> 
>>> 2020年3月16日 下午2:23，Prabhat Sahu >> > 写道：
>>> 
>>> Hi Wenjing,
>>> Please check the below scenario, where the Foreign table on GTT not showing 
>>> records.
>>> 
>>> postgres=# create extension postgres_fdw;
>>> CREATE EXTENSION
>>> postgres=# do $d$
>>> begin
>>> execute $$create server fdw foreign data wrapper postgres_fdw 
>>> options (host 'localhost',dbname 'postgres',port 
>>> '$$||current_setting('port')||$$')$$;
>>> end;
>>> $d$;
>>> DO
>>> postgres=# create user mapping for public server fdw;
>>> CREATE USER MAPPING
>>> 
>>> postgres=# create table lt1 (c1 integer, c2 varchar(50));
>>> CREATE TABLE
>>> postgres=# insert into lt1 values (1,'c21');
>>> INSERT 0 1
>>> postgres=# create foreign table ft1 (c1 integer, c2 varchar(50)) server fdw 
>>> options (table_name 'lt1');
>>> CREATE FOREIGN TABLE
>>> postgres=# select * from ft1;
>>>  c1 | c2  
>>> +-
>>>   1 | c21
>>> (1 row)
>>> 
>>> postgres=# create global temporary table gtt1 (c1 integer, c2 varchar(50));
>>> CREATE TABLE
>>> postgres=# insert into gtt1 values (1,'gtt_c21');
>>> INSERT 0 1
>>> postgres=# create foreign table f_gtt1 (c1 integer, c2 varchar(50)) server 
>>> fdw options (table_name 'gtt1');
>>> CREATE FOREIGN TABLE
>>> 
>>> postgres=# select * from gtt1;
>>>  c1 |   c2
>>> +-
>>>   1 | gtt_c21
>>> (1 row)
>>> 
>>> postgres=# select * from f_gtt1;
>>>  c1 | c2 
>>> +
>>> (0 rows)
>>> 
>>> -- 
>> 
>> I understand that postgre_fdw works similar to dblink.
>> postgre_fdw access to the table requires a new connection.
>> The data in the GTT table is empty in the newly established connection.
>> Because GTT shares structure but not data between connections.
>> 
>> Try local temp table:
>> create temporary table ltt1 (c1 integer, c2 varchar(50));
>> 
>> insert into ltt1 values (1,'gtt_c21');
>> 
>> create foreign table f_ltt1 (c1 integer, c2 varchar(50)) server fdw options 
>> (table_name 'ltt1');
>> 
>> select * from ltt1;
>>  c1 |   c2
>> +-
>>   1 | gtt_c21
>> (1 row)
>> 
>> select * from l_gtt1;
>> ERROR:  relation "l_gtt1" does not exist
>> LINE 1: select * from l_gtt1;
>> 
>> 
>> Wenjing
>> 
>> 
>>> With Regards,
>>> Prabhat Kumar Sahu
>>> EnterpriseDB: http://www.enterprisedb.com 
>> 
> 
> -- 
> regards,tushar
> EnterpriseDB  https://www.enterprisedb.com/ 
> The Enterprise PostgreSQL Company

Re: [Proposal] Global temporary tables

[Prabhat Sahu]

On Mon, Mar 16, 2020 at 1:30 PM Konstantin Knizhnik <
k.knizh...@postgrespro.ru> wrote:

>
> It seems to be expected behavior: GTT data is private to the session and
> postgres_fdw establish its own session where content of the table is empty.
> But if you insert some data in f_gtt1, then you will be able to select
> this data from it because of connection cache in postgres_fdw.
>

Thanks for the explanation.
I am able to insert and select the value from f_gtt1.

 postgres=# insert into f_gtt1 values (1,'gtt_c21');
INSERT 0 1
postgres=# select * from f_gtt1;
 c1 |   c2
+-
  1 | gtt_c21
(1 row)

I have one more doubt,
As you told above "GTT data is private to the session and postgres_fdw
establish its own session where content of the table is empty."
Please check the below scenario,
we can select data from the "root GTT" and "foreign GTT partitioned table"
but we are unable to select data from "GTT partitioned table"

postgres=# create global temporary table gtt2 (c1 integer, c2 integer)
partition by range(c1);
CREATE TABLE
postgres=# create global temporary table gtt2_p1 (c1 integer, c2 integer);
CREATE TABLE
postgres=# create foreign table f_gtt2_p1 (c1 integer, c2 integer) server
fdw options (table_name 'gtt2_p1');
CREATE FOREIGN TABLE
postgres=# alter table gtt2 attach partition f_gtt2_p1 for values from
(minvalue) to (10);
ALTER TABLE
postgres=# insert into gtt2 select i,i from generate_series(1,5,2)i;
INSERT 0 3
postgres=# select * from gtt2;
 c1 | c2
+
  1 |  1
  3 |  3
  5 |  5
(3 rows)

postgres=# select * from gtt2_p1;
 c1 | c2
+
(0 rows)

postgres=# select * from f_gtt2_p1;
 c1 | c2
+
  1 |  1
  3 |  3
  5 |  5
(3 rows)

Is this an expected behavior?

-- 

With Regards,
Prabhat Kumar Sahu
EnterpriseDB: http://www.enterprisedb.com

Re: [HACKERS] Moving relation extension locks out of heavyweight lock manager

[Dilip Kumar]

On Mon, Mar 16, 2020 at 11:56 AM Kuntal Ghosh
 wrote:
>
> On Mon, Mar 16, 2020 at 9:43 AM Dilip Kumar  wrote:
> > On Mon, Mar 16, 2020 at 8:57 AM Masahiko Sawada
> >  wrote:
> > > IsRelationExtensionLockHeld and IsPageLockHeld are used only when
> > > assertion is enabled. So how about making CheckAndSetLockHeld work
> > > only if USE_ASSERT_CHECKING to avoid overheads?
> >
> > That makes sense to me so updated the patch.
> +1
>
> In v10-0001-Assert-that-we-don-t-acquire-a-heavyweight-lock-.patch,
>
> + * Indicate that the lock is released for a particular type of locks.
> s/lock is/locks are

Done

> + /* Indicate that the lock is acquired for a certain type of locks. */
> s/lock is/locks are

Done

>
> In v10-0002-*.patch,
>
> + * Flag to indicate if the page lock is held by this backend.  We don't
> + * acquire any other heavyweight lock while holding the page lock except for
> + * relation extension.  However, these locks are never taken in reverse order
> + * which implies that page locks will also never participate in the deadlock
> + * cycle.
> s/while holding the page lock except for relation extension/while
> holding the page lock except for relation extension and page lock

Done

> + * We don't acquire any other heavyweight lock while holding the page lock
> + * except for relation extension lock.
> Same as above

Done

>
> Other than that, the patches look good to me. I've also done some
> testing after applying the Test-group-deadlock patch provided by Amit
> earlier in the thread. It works as expected.

Thanks for testing.

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


v11-0001-Assert-that-we-don-t-acquire-a-heavyweight-lock-.patch
Description: Binary data


v11-0004-Allow-page-lock-to-conflict-among-parallel-group.patch
Description: Binary data


v11-0003-Allow-relation-extension-lock-to-conflict-among-.patch
Description: Binary data


v11-0002-Add-assert-to-ensure-that-page-locks-don-t-parti.patch
Description: Binary data

Re: [Proposal] Global temporary tables

[曾文旌(义从)]

> 2020年3月16日 下午5:31，Prabhat Sahu  写道：
> 
> 
> 
> On Mon, Mar 16, 2020 at 1:30 PM Konstantin Knizhnik 
> mailto:k.knizh...@postgrespro.ru>> wrote:
> 
> It seems to be expected behavior: GTT data is private to the session and 
> postgres_fdw establish its own session where content of the table is empty.
> But if you insert some data in f_gtt1, then you will be able to select this 
> data from it because of connection cache in postgres_fdw.
> 
> Thanks for the explanation.
> I am able to insert and select the value from f_gtt1.
> 
>  postgres=# insert into f_gtt1 values (1,'gtt_c21');
> INSERT 0 1
> postgres=# select * from f_gtt1;
>  c1 |   c2
> +-
>   1 | gtt_c21
> (1 row)
> 
> I have one more doubt,
> As you told above "GTT data is private to the session and postgres_fdw 
> establish its own session where content of the table is empty."
> Please check the below scenario, 
> we can select data from the "root GTT" and "foreign GTT partitioned table" 
> but we are unable to select data from "GTT partitioned table"
postgres=# select pg_backend_pid();
 pg_backend_pid 

 119135
(1 row)

postgres=# select * from pg_gtt_attached_pids;
 schemaname | tablename | relid |  pid   
+---+---+
 public | gtt2_p1   | 73845 | 119135
 public | gtt2_p1   | 73845 |  51482
(2 rows)


postgres=# select datid,datname,pid,application_name,query from 
pg_stat_activity where usename = ‘wenjing';
 datid | datname  |  pid   | application_name | 
   query 
---+--++--+--
 13589 | postgres | 119135 | psql | select 
datid,datname,pid,application_name,query from pg_stat_activity where usename = 
'wenjing';
 13589 | postgres |  51482 | postgres_fdw | COMMIT TRANSACTION
(2 rows)

This can be explained
The postgre_fdw connection has not been disconnected, and it produced data in 
another session.
In other words, gtt2_p1 is empty in session 119135, but not in session 51482.


> 
> postgres=# create global temporary table gtt2 (c1 integer, c2 integer) 
> partition by range(c1);
> CREATE TABLE
> postgres=# create global temporary table gtt2_p1 (c1 integer, c2 integer);
> CREATE TABLE
> postgres=# create foreign table f_gtt2_p1 (c1 integer, c2 integer) server fdw 
> options (table_name 'gtt2_p1');
> CREATE FOREIGN TABLE
> postgres=# alter table gtt2 attach partition f_gtt2_p1 for values from 
> (minvalue) to (10);
> ALTER TABLE
> postgres=# insert into gtt2 select i,i from generate_series(1,5,2)i;
> INSERT 0 3
> postgres=# select * from gtt2;
>  c1 | c2 
> +
>   1 |  1
>   3 |  3
>   5 |  5
> (3 rows)
> 
> postgres=# select * from gtt2_p1;
>  c1 | c2 
> +
> (0 rows)
> 
> postgres=# select * from f_gtt2_p1;
>  c1 | c2 
> +
>   1 |  1
>   3 |  3
>   5 |  5
> (3 rows)
> 
> Is this an expected behavior?
> 
> -- 
> With Regards,
> Prabhat Kumar Sahu
> EnterpriseDB: http://www.enterprisedb.com 

Re: backup manifests

[tushar]

On 3/14/20 2:04 AM, Robert Haas wrote:

OK. Done in the attached version


Thanks. Verified.

--
regards,tushar
EnterpriseDB  https://www.enterprisedb.com/
The Enterprise PostgreSQL Company

Re: v13 latest snapshot build error

[Devrim Gündüz]

On Wed, 2020-03-11 at 22:52 -0400, Tom Lane wrote:
> Artur Zakirov  writes:
> > I'm not familiar with the patch itself. But I think there is just a lack 
> > of the comma here, after ", /tmp" :-)
> 
> [ blink... ]  There definitely is a comma there in the version of the
> patch that's in the Fedora repo.


Exactly.Looks like I broke the patch while trying to update the patch.

Sorry for the noise.

Regards,

-- 
Devrim Gündüz
Open Source Solution Architect, Red Hat Certified Engineer
Twitter: @DevrimGunduz , @DevrimGunduzTR


signature.asc
Description: This is a digitally signed message part

Re: [PATCH] Use PKG_CHECK_MODULES to detect the libxml2 library

[Hugh McMaster]

On Sat, 14 Mar 2020 at 03:06, Tom Lane wrote:
> Looking again at the generated configure code, I realize I shouldn't have
> left off the ACTION-IF-NOT-FOUND argument --- the default is to
> throw an error, but we'd rather fall through and try to use xml2-config.
> The eventual AC_CHECK_LIB(xml2, ...) test will catch the situation
> where the library isn't there.  Updated patch attached.

The updated patch works fine. Thanks for working on this!

Hugh

Re: Replication & recovery_min_apply_delay

[Asim R P]

A key challenge here is how to determine the starting point for WAL
receiver when the startup process starts it while still replaying WAL
that's already received.  Hao and I wrote a much faster and less intrusive
solution to determine the starting point.  Scan the first page of each WAL
segment file, starting from the one that's currently being read for
replay.  If the first page is found valid, move to the next segment file
and check its first page.  Continue this one segment file at a time until
either the segment file doesn't exist or the page is not valid.  The
starting point is then the previous segment's start address.

There is no need to read till the end of WAL, one record at a time, like
the original proposal upthread did.  The most recently flushed LSN does not
need to be persisted on disk.

Please see attached patch which also contains a TAP test to induce replay
lag and validate the fix.

Asim


v3-0001-Start-WAL-receiver-before-startup-process-replays.patch
Description: Binary data

Re: [PATCH] Connection time for \conninfo

[David Steele]

On 3/14/20 3:16 PM, Rodrigo Ramírez Norambuena wrote:


I forgot about that ... It passed a little time from my new pushed
changes. So go ahead :)


This patch has been returned with feedback. Please feel free to resubmit 
in a future CF when you believe the feedback has been addressed.


Regards,
--
-David
da...@pgmasters.net

Re: Berserk Autovacuum (let's save next Mandrill)

[Justin Pryzby]

On Mon, Mar 16, 2020 at 12:53:43PM +0900, Masahiko Sawada wrote:

> There is already a consensus on introducing new 2 parameters, but as
> the second idea I'd like to add one (or two) GUC(s) to my suggestion,
> say autovacuum_vacuum_freeze_insert_ratio; this parameter is the ratio
> of the number of inserted tuples for total number of tuples modified
> and inserted, in order to trigger insert-only vacuum. For example,
> suppose the table has 1,000,000 tuples and we set threshold = 0,
> scale_factor = 0.2 and freeze_insert_ratio = 0.9, we will trigger
> normal autovacuum when n_dead_tup + n_ins_since_vacuum > 200,000, but
> we will instead trigger insert-only autovacuum, which is a vacuum with
> vacuum_freeze_min_age = 0, when n_ins_since_vacuum > 180,000 (=200,000
> * 0.9). IOW if 90% of modified tuples are insertions, we freeze tuples
> aggressively. If we want to trigger insert-only vacuum only on
> insert-only table we can set freeze_insert_ratio = 1.0. The down side
> of this idea is that we cannot disable autovacuum triggered by the
> number of inserted, although we might be able to introduce more one
> GUC that controls whether to include the number of inserted tuples for
> triggering autovacuum (say, autovacuum_vacuum_triggered_by_insert =
> on|off). The pros of this idea would be that we can ensure that
> insert-only vacuum will run only in the case where the ratio of
> insertion is large enough.

I was thinking about something like this myself.  I would appreciate keeping
separate the thresholds for 1) triggering vacuum; and, 2) the options
autovacuum uses when it runs (in this case, FREEZE).  Someone might want
autovacuum to run with FREEZE on a table vacuumed due to dead tuples (say, on a
partitioned table), or might *not* want to run FREEZE on a table vacuumed due
to insertions (maybe because index scans are too expensive or FREEZE makes it
too slow).

Normally, when someone complains about bad plan related to no index-onlyscan,
we tell them to run vacuum, and if that helps, then ALTER TABLE .. SET
(autovacuum_vacuum_scale_factor=0.005).

If there's two thresholds (4 GUCs and 4 relopts) for autovacuum, then do we
have to help determine which one was being hit, and which relopt to set?

I wonder if the new insert GUCs should default to -1 (disabled)?  And the
insert thresholds should be set by new insert relopt (if set), or by new insert
GUC (default -1), else normal relopt, or normal GUC.  The defaults would give
50 + 0.20*n.  When someone asks about IOS, we'd tell them to set
autovacuum_vacuum_scale_factor=0.005, same as now.

vac_ins_scale_factor =
(relopts && relopts->vacuum_ins_scale_factor >= 0) ? 
relopts->vacuum_ins_scale_factor :
autovacuum_vac_ins_scale >= 0 ? autovacuum_vac_ins_scale : 
(relopts && relopts->vacuum_scale_factor >= 0) ? 
relopts->vacuum_scale_factor :
autovacuum_vac_scale;

One would disable autovacuum triggered by insertions by setting
autovacuum_vacuum_insert_scale_factor=1e10 (which I think should also be the
max for this patch).

It seems to me that the easy thing to do is to implement this initially without
FREEZE (which is controlled by vacuum_freeze_table_age), and defer until
July/v14 further discussion and implementation of another GUC/relopt for
autovacuum freezing to be controlled by insert thresholds (or ratio).

-- 
Justin

Re: adding partitioned tables to publications

[Peter Eisentraut]

I was trying to extract some preparatory work from the remaining patches 
and came up with the attached.  This is part of your patch 0003, but 
also relevant for part 0004.  The problem was that COPY (SELECT *) is 
not sufficient when the table has generated columns, so we need to build 
the column list explicitly.


Thoughts?

--
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From c52672ff1de4d7c55c4b50eb7986f45801ea4c60 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Mon, 16 Mar 2020 13:27:51 +0100
Subject: [PATCH] Prepare to support non-tables in publications

This by itself doesn't change any functionality but prepares the way
for having relations other than base tables in publications.

Make arrangements for COPY handling the initial table sync.  For
non-tables we have to use COPY (SELECT ...) instead of directly
copying from the table, but then we have to take care to omit
generated columns from the column list.

Also, remove a hardcoded reference to relkind = 'r' and rely on
pg_relation_is_publishable(), which matches what the publisher can
actually publish and is correct even in cross-version scenarios.

Discussion: 
https://www.postgresql.org/message-id/flat/CA+HiwqH=Y85vRK3mOdjEkqFK+E=ST=eQiHdpj43L=_ejmoo...@mail.gmail.com
---
 src/backend/replication/logical/tablesync.c | 43 -
 src/include/replication/logicalproto.h  |  1 +
 2 files changed, 35 insertions(+), 9 deletions(-)

diff --git a/src/backend/replication/logical/tablesync.c 
b/src/backend/replication/logical/tablesync.c
index 98825f01e9..358b0a3726 100644
--- a/src/backend/replication/logical/tablesync.c
+++ b/src/backend/replication/logical/tablesync.c
@@ -639,26 +639,27 @@ fetch_remote_table_info(char *nspname, char *relname,
WalRcvExecResult *res;
StringInfoData cmd;
TupleTableSlot *slot;
-   Oid tableRow[2] = {OIDOID, CHAROID};
-   Oid attrRow[4] = {TEXTOID, OIDOID, INT4OID, 
BOOLOID};
+   Oid tableRow[] = {OIDOID, CHAROID, CHAROID, 
BOOLOID};
+   Oid attrRow[] = {TEXTOID, OIDOID, INT4OID, BOOLOID};
boolisnull;
int natt;
+   boolremote_is_publishable;
 
lrel->nspname = nspname;
lrel->relname = relname;
 
/* First fetch Oid and replica identity. */
initStringInfo(&cmd);
-   appendStringInfo(&cmd, "SELECT c.oid, c.relreplident"
+   appendStringInfo(&cmd, "SELECT c.oid, c.relreplident, c.relkind,"
+"pg_relation_is_publishable(c.oid)"
 "  FROM pg_catalog.pg_class c"
 "  INNER JOIN pg_catalog.pg_namespace 
n"
 "ON (c.relnamespace = n.oid)"
 " WHERE n.nspname = %s"
-"   AND c.relname = %s"
-"   AND c.relkind = 'r'",
+"   AND c.relname = %s",
 quote_literal_cstr(nspname),
 quote_literal_cstr(relname));
-   res = walrcv_exec(wrconn, cmd.data, 2, tableRow);
+   res = walrcv_exec(wrconn, cmd.data, lengthof(tableRow), tableRow);
 
if (res->status != WALRCV_OK_TUPLES)
ereport(ERROR,
@@ -675,6 +676,13 @@ fetch_remote_table_info(char *nspname, char *relname,
Assert(!isnull);
lrel->replident = DatumGetChar(slot_getattr(slot, 2, &isnull));
Assert(!isnull);
+   lrel->relkind = DatumGetChar(slot_getattr(slot, 3, &isnull));
+   Assert(!isnull);
+   remote_is_publishable = DatumGetBool(slot_getattr(slot, 4, &isnull));
+   if (isnull || !remote_is_publishable)
+   ereport(ERROR,
+   (errmsg("table \"%s.%s\" on the publisher is 
not publishable",
+   nspname, relname)));
 
ExecDropSingleTupleTableSlot(slot);
walrcv_clear_result(res);
@@ -696,7 +704,7 @@ fetch_remote_table_info(char *nspname, char *relname,
 lrel->remoteid,
 (walrcv_server_version(wrconn) >= 
12 ? "AND a.attgenerated = ''" : ""),
 lrel->remoteid);
-   res = walrcv_exec(wrconn, cmd.data, 4, attrRow);
+   res = walrcv_exec(wrconn, cmd.data, lengthof(attrRow), attrRow);
 
if (res->status != WALRCV_OK_TUPLES)
ereport(ERROR,
@@ -765,8 +773,25 @@ copy_table(Relation rel)
 
/* Start copy on the publisher. */
initStringInfo(&cmd);
-   appendStringInfo(&cmd, "COPY %s TO STDOUT",
-
quot

Re: WIP/PoC for parallel backup

[Rajkumar Raghuwanshi]

Hi Asif,

On testing further, I found when taking backup with -R, pg_basebackup
crashed
this crash is not consistently reproducible.

[edb@localhost bin]$ ./psql postgres -p 5432 -c "create table test (a
text);"
CREATE TABLE
[edb@localhost bin]$ ./psql postgres -p 5432 -c "insert into test values
('parallel_backup with -R recovery-conf');"
INSERT 0 1
[edb@localhost bin]$ ./pg_basebackup -p 5432 -j 2 -D /tmp/test_bkp/bkp -R
Segmentation fault (core dumped)

stack trace looks the same as it was on earlier reported crash with
tablespace.
--stack trace
[edb@localhost bin]$ gdb -q -c core.37915 pg_basebackup
Loaded symbols for /lib64/libnss_files.so.2
Core was generated by `./pg_basebackup -p 5432 -j 2 -D /tmp/test_bkp/bkp
-R'.
Program terminated with signal 11, Segmentation fault.
#0  0x004099ee in worker_get_files (wstate=0xc1e458) at
pg_basebackup.c:3175
3175 backupinfo->curr = fetchfile->next;
Missing separate debuginfos, use: debuginfo-install
keyutils-libs-1.4-5.el6.x86_64 krb5-libs-1.10.3-65.el6.x86_64
libcom_err-1.41.12-24.el6.x86_64 libselinux-2.0.94-7.el6.x86_64
openssl-1.0.1e-58.el6_10.x86_64 zlib-1.2.3-29.el6.x86_64
(gdb) bt
#0  0x004099ee in worker_get_files (wstate=0xc1e458) at
pg_basebackup.c:3175
#1  0x00408a9e in worker_run (arg=0xc1e458) at pg_basebackup.c:2715
#2  0x003921a07aa1 in start_thread (arg=0x7f72207c0700) at
pthread_create.c:301
#3  0x0039212e8c4d in clone () at
../sysdeps/unix/sysv/linux/x86_64/clone.S:115
(gdb)

Thanks & Regards,
Rajkumar Raghuwanshi


On Mon, Mar 16, 2020 at 2:14 PM Jeevan Chalke <
jeevan.cha...@enterprisedb.com> wrote:

> Hi Asif,
>
>
>> Thanks Rajkumar. I have fixed the above issues and have rebased the patch
>> to the latest master (b7f64c64).
>> (V9 of the patches are attached).
>>
>
> I had a further review of the patches and here are my few observations:
>
> 1.
> +/*
> + * stop_backup() - ends an online backup
> + *
> + * The function is called at the end of an online backup. It sends out
> pg_control
> + * file, optionally WAL segments and ending WAL location.
> + */
>
> Comments seem out-dated.
>
> 2. With parallel jobs, maxrate is now not supported. Since we are now
> asking
> data in multiple threads throttling seems important here. Can you please
> explain why have you disabled that?
>
> 3. As we are always fetching a single file and as Robert suggested, let
> rename
> SEND_FILES to SEND_FILE instead.
>
> 4. Does this work on Windows? I mean does pthread_create() work on Windows?
> I asked this as I see that pgbench has its own implementation for
> pthread_create() for WIN32 but this patch doesn't.
>
> 5. Typos:
> tablspace => tablespace
> safly => safely
>
> 6. parallel_backup_run() needs some comments explaining the states it goes
> through PB_* states.
>
> 7.
> +case PB_FETCH_REL_FILES:/* fetch files from server */
> +if (backupinfo->activeworkers == 0)
> +{
> +backupinfo->backupstate = PB_STOP_BACKUP;
> +free_filelist(backupinfo);
> +}
> +break;
> +case PB_FETCH_WAL_FILES:/* fetch WAL files from server */
> +if (backupinfo->activeworkers == 0)
> +{
> +backupinfo->backupstate = PB_BACKUP_COMPLETE;
> +}
> +break;
>
> Why free_filelist() is not called in PB_FETCH_WAL_FILES case?
>
> Thanks
> --
> Jeevan Chalke
> Associate Database Architect & Team Lead, Product Development
> EnterpriseDB Corporation
> The Enterprise PostgreSQL Company
>
> Phone: +91 20 66449694
>
> Website: www.enterprisedb.com
> EnterpriseDB Blog: http://blogs.enterprisedb.com/
> Follow us on Twitter: http://www.twitter.com/enterprisedb
>
> This e-mail message (and any attachment) is intended for the use of the
> individual or entity to whom it is addressed. This message contains
> information from EnterpriseDB Corporation that may be privileged,
> confidential, or exempt from disclosure under applicable law. If you are
> not the intended recipient or authorized to receive this for the intended
> recipient, any use, dissemination, distribution, retention, archiving, or
> copying of this communication is strictly prohibited. If you have received
> this e-mail in error, please notify the sender immediately by reply e-mail
> and delete this message.
>

Re: improve transparency of bitmap-only heap scans

[James Coleman]

On Tue, Mar 10, 2020 at 12:15 PM David Steele  wrote:
>
> Hi Jeff,
>
> On 2/7/20 10:22 AM, Alexey Bashtanov wrote:
> > I've changed it all to "unfetched" for at least not to call the same
> > thing differently
> > in the code and in the output, and also rebased it and fit in 80 lines
> > width limit.
>
> What do you think of Alexey's updates?
>
> Regards,
> --
> -David
> da...@pgmasters.net

I've added myself as a reviewer.

The patch looks good to me. It doesn't seem to have much risk either;
there are not spec concerns applicable (since it's EXPLAIN), and the
surface area for impact quite small. Both make check and check-world
pass.

Here's a test query setup I worked up:

create table exp(a int, d int);
insert into exp(a, d) select random() * 100, t.i % 50 from
generate_series(0,1000) t(i);
create index index_exp_a on exp(a);
create index index_exp_d on exp(d);
analyze exp;

Then:
explain analyze select count(*) from exp where a = 25 and d between 5 and 10;
shows: Heap Blocks: exact=10518

but if I:
vacuum freeze exp;
then it shows: Heap Blocks: unfetched=10518
as I'd expect.

One question though: if I change the query to:
explain (analyze, buffers) select count(*) from exp where a between 50
and 100 and d between 5 and 10;
then I get a parallel bitmap heap scan, and I only see exact heap
blocks (see attached explain output).

Does the original optimization cover parallel bitmap heap scans like
this? If not, I think this patch is likely ready for committer. If so,
then we still need support for stats tracking and explain output for
parallel nodes.

I've taken the liberty of:
- Reformatting slightly for a cleaner diff.
- Running pgindent against the changes
- Added a basic commit message.
- Add unfetched_pages initialization to ExecInitBitmapHeapScan.

See attached.

Thanks,
James
From 95babe8447eadb40c2d1452a9102d4766269d80f Mon Sep 17 00:00:00 2001
From: James Coleman 
Date: Sun, 15 Mar 2020 20:27:19 -0400
Subject: [PATCH v3] Show bitmap only unfetched page count to EXPLAIN

7c70996ebf0949b142a99 added an optimization to bitmap heap scans to
avoid fetching the heap page where the visibility map makes that
possible, much like index only scans.

However that commit didn't add this output to EXPLAIN, so it's not
obvious when the optimization is kicking in and when it's not. So, track
the number of skipped pages and report it in EXPLAIN output.

Author: Jeff Janes
Reviewers: Emre Hasegeli, Alexey Bashtanov, James Coleman
---
 src/backend/commands/explain.c| 8 +++-
 src/backend/executor/nodeBitmapHeapscan.c | 5 +++--
 src/include/nodes/execnodes.h | 2 ++
 3 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index d901dc4a50..5a5e04ecbb 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -2777,6 +2777,8 @@ show_tidbitmap_info(BitmapHeapScanState *planstate, ExplainState *es)
 {
 	if (es->format != EXPLAIN_FORMAT_TEXT)
 	{
+		ExplainPropertyInteger("Unfetched Heap Blocks", NULL,
+			   planstate->unfetched_pages, es);
 		ExplainPropertyInteger("Exact Heap Blocks", NULL,
 			   planstate->exact_pages, es);
 		ExplainPropertyInteger("Lossy Heap Blocks", NULL,
@@ -2784,10 +2786,14 @@ show_tidbitmap_info(BitmapHeapScanState *planstate, ExplainState *es)
 	}
 	else
 	{
-		if (planstate->exact_pages > 0 || planstate->lossy_pages > 0)
+		if (planstate->exact_pages > 0 || planstate->lossy_pages > 0
+			|| planstate->unfetched_pages > 0)
 		{
 			ExplainIndentText(es);
 			appendStringInfoString(es->str, "Heap Blocks:");
+			if (planstate->unfetched_pages > 0)
+appendStringInfo(es->str, " unfetched=%ld",
+ planstate->unfetched_pages);
 			if (planstate->exact_pages > 0)
 appendStringInfo(es->str, " exact=%ld", planstate->exact_pages);
 			if (planstate->lossy_pages > 0)
diff --git a/src/backend/executor/nodeBitmapHeapscan.c b/src/backend/executor/nodeBitmapHeapscan.c
index ae8a11da30..dce955636f 100644
--- a/src/backend/executor/nodeBitmapHeapscan.c
+++ b/src/backend/executor/nodeBitmapHeapscan.c
@@ -231,14 +231,14 @@ BitmapHeapNext(BitmapHeapScanState *node)
  * node->return_empty_tuples.
  */
 node->return_empty_tuples = tbmres->ntuples;
+node->unfetched_pages++;
 			}
 			else if (!table_scan_bitmap_next_block(scan, tbmres))
 			{
 /* AM doesn't think this block is valid, skip */
 continue;
 			}
-
-			if (tbmres->ntuples >= 0)
+			else if (tbmres->ntuples >= 0)
 node->exact_pages++;
 			else
 node->lossy_pages++;
@@ -734,6 +734,7 @@ ExecInitBitmapHeapScan(BitmapHeapScan *node, EState *estate, int eflags)
 	scanstate->pvmbuffer = InvalidBuffer;
 	scanstate->exact_pages = 0;
 	scanstate->lossy_pages = 0;
+	scanstate->unfetched_pages = 0;
 	scanstate->prefetch_iterator = NULL;
 	scanstate->prefetch_pages = 0;
 	scanstate->prefetch_target = 0;
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index cd3

Re: Online checksums verification in the backend

[Julien Rouhaud]

On Mon, Mar 16, 2020 at 09:42:39AM +0100, Julien Rouhaud wrote:
> On Mon, Mar 16, 2020 at 01:53:35PM +0900, Masahiko Sawada wrote:
> >
> > In addition to comments from Michael-san, here are my comments:

Thanks both for the reviews.  I'm attaching a v3 with all comments addressed,
except:

> It seems to me that this test would be a good fit for
> src/test/modules/test_misc/.


AFAICT this is explicitly documented as tests for various extensions, and for
now it's a core function, so I didn't move it.


> +Run
> +make check
> +or
> +make installcheck
> Why is installcheck mentioned here?


This is actually already used in multiple other test readme.
>From d96c67e2e591cecca63fdf8c9d808bb6a1b72866 Mon Sep 17 00:00:00 2001
From: Julien Rouhaud 
Date: Mon, 4 Nov 2019 08:40:23 +0100
Subject: [PATCH v3] Add a pg_check_relation() function.

This functions checks the validity of the checksums for all non-dirty blocks of
a given relation, and optionally a given fork, and returns the list of all
blocks that don't match, along with the expected and found checksums.

Author: Julien Rouhaud
Reviewed-by: Michael Paquier, Masahiko Sawada
Discussion: 
https://postgr.es/m/CAOBaU_aVvMjQn%3Dge5qPiJOPMmOj5%3Dii3st5Q0Y%2BWuLML5sR17w%40mail.gmail.com
---
 doc/src/sgml/func.sgml|  44 ++
 src/backend/catalog/system_views.sql  |   7 +
 src/backend/storage/page/checksum.c   | 427 +-
 src/backend/utils/adt/Makefile|   1 +
 src/backend/utils/adt/checksumfuncs.c |  96 
 src/backend/utils/init/globals.c  |   8 +
 src/backend/utils/misc/guc.c  |  43 ++
 src/include/catalog/pg_proc.dat   |   8 +
 src/include/miscadmin.h   |   8 +
 src/include/storage/checksum.h|   7 +
 src/include/utils/guc_tables.h|   1 +
 src/test/Makefile |   3 +-
 src/test/check_relation/.gitignore|   2 +
 src/test/check_relation/Makefile  |  23 +
 src/test/check_relation/README|  23 +
 .../check_relation/t/01_checksums_check.pl| 274 +++
 16 files changed, 971 insertions(+), 4 deletions(-)
 create mode 100644 src/backend/utils/adt/checksumfuncs.c
 create mode 100644 src/test/check_relation/.gitignore
 create mode 100644 src/test/check_relation/Makefile
 create mode 100644 src/test/check_relation/README
 create mode 100644 src/test/check_relation/t/01_checksums_check.pl

diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 323366feb6..5847d7f655 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -21815,6 +21815,50 @@ SELECT (pg_stat_file('filename')).modification;
 
   
 
+  
+   Data Sanity Functions
+
+   
+The functions shown in 
+provide means to check for health of data file in a cluster.
+   
+
+   
+Data Sanity Functions
+
+ 
+  Name Return Type 
Description
+  
+ 
+
+ 
+  
+   
+pg_check_relation(relation 
oid, fork 
text)
+   
+   setof record
+   Validate the checksums for all blocks of a given relation, and
+   optionally the given fork.
+  
+ 
+
+   
+
+   
+pg_check_relation
+   
+   
+pg_check_relation iterates over all the blocks of all
+or the specified fork of a given relation and verify their checksum.  It
+returns the list of blocks for which the found checksum doesn't match the
+expected one.  You must be a member of the
+pg_read_all_stats role to use this function.  It can
+only be used if data checksums are enabled.  See  for more information.
+   
+
+  
+
   
 
   
diff --git a/src/backend/catalog/system_views.sql 
b/src/backend/catalog/system_views.sql
index b8a3f46912..e266292b03 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -1386,6 +1386,13 @@ LANGUAGE INTERNAL
 STRICT STABLE PARALLEL SAFE
 AS 'jsonb_path_query_first_tz';
 
+CREATE OR REPLACE FUNCTION pg_check_relation(
+  IN relation regclass DEFAULT NULL::regclass, IN fork text DEFAULT NULL::text,
+  OUT relid oid, OUT forknum integer, OUT failed_blocknum bigint,
+  OUT expected_checksum integer, OUT found_checksum integer)
+  RETURNS SETOF record VOLATILE LANGUAGE internal AS 'pg_check_relation'
+  PARALLEL RESTRICTED;
+
 --
 -- 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/storage/page/checksum.c 
b/src/backend/storage/page/checksum.c
index e010691c9f..aec5e2ad2d 100644
--- a/src/backend/storage/page/checksum.c
+++ b/src/backend/storage/page/checksum.c
@@ -15,8 +15,429 @@
 
 #include "storage/checksum.h"
 /*
- * The actual code is in storage/checksum_impl.h.  This is done so that
- * external programs can incorporate the checksum code by #include'ing
- * that file from the exported Postgres headers.  (Compare our CRC co

Re: [PATCH] kNN for btree

[Alexander Korotkov]

On Wed, Mar 4, 2020 at 2:39 PM Alexander Korotkov
 wrote:
> On Wed, Mar 4, 2020 at 4:58 AM Peter Geoghegan  wrote:
> > On Mon, Mar 2, 2020 at 1:27 PM Alexander Korotkov
> >  wrote:
> > > I've rebased the patchset to the current master and made some
> > > refactoring.  I hope it would be possible to bring it to committable
> > > shape during this CF.  This need more refactoring though.
> >
> > This patch doesn't change anything about the B-Tree on-disk format -- right?
>
> Yes, this is correct.  No on-disk format changes, just new scanning strategy.

After another try to polish this patch I figured out that the way it's
implemented is unnatural.  I see the two reasonable ways to implement
knn for B-tree, but current implementation matches none of them.

1) Implement knn as two simultaneous scans over B-tree: forward and
backward.  It's similar to what current patchset does.  But putting
this logic to B-tree seems artificial.  What B-tree does here is still
unidirectional scan.  On top of that we merge results of two
unidirectional scans.  The appropriate place to do this high-level
work is IndexScan node or even Optimizer/Executor (Merge node over to
IndexScan nodes), but not B-tree itself.
2) Implement arbitrary scans in B-tree using priority queue like GiST
and SP-GiST do.  That would lead to much better support for KNN.  We
can end up in supporting interesting cases like "ORDER BY col1 DESC,
col2 <> val1, col2 ASC" or something.  But that's requires way more
hacking in B-tree core.

So, I'm marking this patch RWF.  We should try re-implement this for v14.

--
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: Libpq support to connect to standby server as priority

[David Steele]

On 2/28/20 11:05 AM, Alvaro Herrera wrote:

MauMau, Greg, is any of you submitting a new patch for this?


This patch has not had any updates in months and now we are halfway 
through the CF so I have marked it Returned with Feedback.


If a patch arrives soon I'll be happy to revive the entry, otherwise 
please submit to a future CF when a new patch is available.


Regards,
--
-David
da...@pgmasters.net

Re: ALTER tbl rewrite loses CLUSTER ON index

[Justin Pryzby]

On Mon, Mar 16, 2020 at 04:01:42PM +0900, Amit Langote wrote:
> I came across a commit that recently went in:
> 
> commit 1cc9c2412cc9a2fbe6a381170097d315fd40ccca
> Author: Peter Eisentraut 
> Date:   Fri Mar 13 11:28:11 2020 +0100
> 
> Preserve replica identity index across ALTER TABLE rewrite
> 
> which fixes something very similar to what we are trying to with this
> patch.  The way it's done looks to me very close to what you are
> telling.  I have updated the patch to be similar to the above fix.

Yes, I noticed it too.

Should we use your get_index_isclustered more widely ?

Also, should we call it "is_index_clustered", since otherwise it sounds alot
like "+get_index_clustered" (without "is"), which sounds like it takes a table
and returns which index is clustered.  That might be just as useful for some of
these callers.

-- 
Justin
>From add5e8481c70b6b66342b264a243f26f4c634e53 Mon Sep 17 00:00:00 2001
From: Amit Langote 
Date: Mon, 16 Mar 2020 16:01:42 +0900
Subject: [PATCH v7 1/2] ALTER tbl rewrite loses CLUSTER ON index

On Fri, Mar 13, 2020 at 2:19 AM Tom Lane  wrote:
> Justin Pryzby  writes:
> > @cfbot: resending with only Amit's 0001, since Michael pushed a variation on
> > 0002.

Thank you for taking a look at it.

> Boy, I really dislike this patch.  ATPostAlterTypeParse is documented as
> using the supplied definition string, and nothing else, to reconstruct
> the index.  This breaks that without even the courtesy of documenting
> the breakage.  Moreover, the reason why it's designed like that is to
> avoid requiring the old index objects to still be accessible.  So I'm
> surprised that this hack works at all.  I don't think it would have
> worked at the time the code was first written, and I think it's imposing
> a constraint we'll have problems with (again?) in future.

Okay, so maybe in the middle of ATPostAlterTypeParse() is not a place
to do it, but don't these arguments apply to
RebuildConstraintComment(), which I based the patch on?

> The right way to fix this is to note the presence of the indisclustered
> flag when we're examining the index earlier, and include a suitable
> command in the definition string.  So probably pg_get_indexdef_string()
> is what needs to change.  It doesn't look like that's used anywhere
> else, so we can just redefine its behavior as needed.

I came across a commit that recently went in:

commit 1cc9c2412cc9a2fbe6a381170097d315fd40ccca
Author: Peter Eisentraut 
Date:   Fri Mar 13 11:28:11 2020 +0100

Preserve replica identity index across ALTER TABLE rewrite

which fixes something very similar to what we are trying to with this
patch.  The way it's done looks to me very close to what you are
telling.  I have updated the patch to be similar to the above fix.

--
Thank you,
Amit
---
 src/backend/commands/tablecmds.c  | 49 +++
 src/backend/utils/cache/lsyscache.c   | 23 +++
 src/include/utils/lsyscache.h |  1 +
 src/test/regress/expected/alter_table.out | 34 
 src/test/regress/sql/alter_table.sql  | 15 +++
 5 files changed, 122 insertions(+)

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 8c33b67c1b..73d9392878 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -177,6 +177,7 @@ typedef struct AlteredTableInfo
 	List	   *changedIndexOids;	/* OIDs of indexes to rebuild */
 	List	   *changedIndexDefs;	/* string definitions of same */
 	char	   *replicaIdentityIndex;	/* index to reset as REPLICA IDENTITY */
+	char	   *clusterOnIndex;		/* index to CLUSTER ON */
 } AlteredTableInfo;
 
 /* Struct describing one new constraint to check in Phase 3 scan */
@@ -11579,9 +11580,28 @@ RememberReplicaIdentityForRebuilding(Oid indoid, AlteredTableInfo *tab)
 	tab->replicaIdentityIndex = get_rel_name(indoid);
 }
 
+/*
+ * Subroutine for ATExecAlterColumnType: remember any clustered index.
+ */
+static void
+RememberClusterOnForRebuilding(Oid indoid, AlteredTableInfo *tab)
+{
+	if (!get_index_isclustered(indoid))
+		return;
+
+	if (tab->clusterOnIndex)
+		elog(ERROR, "relation %u has multiple clustered indexes", tab->relid);
+
+	tab->clusterOnIndex = get_rel_name(indoid);
+}
+
 /*
  * Subroutine for ATExecAlterColumnType: remember that a constraint needs
  * to be rebuilt (which we might already know).
+ *
+ * For constraint's index (if any), also remember if it's the table's replica
+ * identity or its clustered index, so that ATPostAlterTypeCleanup() can
+ * queue up commands necessary to restore that property.
  */
 static void
 RememberConstraintForRebuilding(Oid conoid, AlteredTableInfo *tab)
@@ -11604,9 +11624,18 @@ RememberConstraintForRebuilding(Oid conoid, AlteredTableInfo *tab)
 		tab->changedConstraintDefs = lappend(tab->changedConstraintDefs,
 			 defstring);
 
+		/*
+		 * For constraint's index (if any), remember if it's the table's
+		 * replica identity or its clustered index, so that
+		 * ATP

Re: VACUUM memory management

[David Steele]

On 1/28/20 1:36 PM, Ibrar Ahmed wrote:
On Wed, Jan 22, 2020 at 11:17 AM k.jami...@fujitsu.com 
I might have missed something more,


but I'll continue reviewing after the rebased patch.

Yes, I am working on that. I will send the rebased and updated patch.


This patch has not had any updates in months and now we are halfway 
through the CF so I have marked it Returned with Feedback.


If a patch arrives soon I'll be happy to revive the entry, otherwise 
please submit to a future CF when a new patch is available.


Regards,
--
-David
da...@pgmasters.net

Re: Parallel leader process info in EXPLAIN

[David Steele]

Hi Thomas,

On 1/26/20 7:03 PM, Thomas Munro wrote:


Fair point.  I will look into that.


Are you still planning on looking at this patch for PG13?

Based on the current state (002 abandoned, 001 needs total rework) I'd 
say it should just be Returned with Feedback or Closed for now.


Regards,
--
-David
da...@pgmasters.net

Re: row filtering for logical replication

[David Steele]

On 3/3/20 12:39 PM, David Steele wrote:

Hi Euler,

On 1/21/20 2:32 AM, Craig Ringer wrote:

On Fri, 17 Jan 2020 at 07:58, Euler Taveira  wrote:


Em qui., 16 de jan. de 2020 às 18:57, Tomas Vondra
 escreveu:


Euler, this patch is still in "waiting on author" since 11/25. Do you
plan to review changes made by Amit in the patches he submitted, or 
what

are your plans with this patch?

Yes, I'm working on Amit suggestions. I'll post a new patch as soon 
as possible.


Great. I think this'd be nice to see.


The last CF for PG13 has started. Do you have a new patch ready?


I have marked this patch Returned with Feedback since no new patch has 
been posted.


Please submit to a future CF when a new patch is available.

Regards,
--
-David
da...@pgmasters.net

[PATCH] Btree BackwardScan race condition on Standby during VACUUM

[Michail Nikolaev]

Hello, hackers.

-- ABSTRACT --
There is a race condition between btree_xlog_unlink_page and _bt_walk_left.
A lot of versions are affected including 12 and new-coming 13.
Happens only on standby. Seems like could not cause invalid query results.

-- REMARK --
While working on support for index hint bits on standby [1] I have
started to getting
"ERROR:  could not find left sibling of block  in index "
during stress tests.

I was sure I have broken something in btree and spent a lot of time
trying to figure what.
And later...  I realized what it is bug in btree since a very old times...
Because of much faster scans with LP_DEAD support on a standby it
happens much more frequently in my case.

-- HOW TO REPRODUCE  --
It is not easy to reproduce the issue but you can try (tested on
REL_12_STABLE and master):

1) Setup master (sync replica and 'remote_apply' are not required -
just make scripts simpler):
autovacuum = off
synchronous_standby_names = '*'
synchronous_commit = 'remote_apply'

2) Setup standby:
primary_conninfo = 'user=postgres host=127.0.0.1 port=5432
sslmode=prefer sslcompression=0 gssencmode=prefer krbsrvname=postgres
target_session_attrs=any'
port = 6543

3) Prepare pgbench file with content (test.bench):
BEGIN;
select * from pgbench_accounts order by aid desc limit 1;
END;

4) Prepare the index:
./pgbench -i -s 10 -U postgres
./psql -U postgres -c "delete from pgbench_accounts where aid IN
(select aid from pgbench_accounts order by aid desc limit 50)"

5) Start index scans on the standby:
./pgbench -f test.bench -j 1 -c ${NUMBER_OF_CORES} -n -P 1 -T 1 -U
postgres -p 6543

6) Run vacuum on the master:
./psql -U postgres -c "vacuum pgbench_accounts"

7) You should see something like this:
> progress: 1.0 s, 5.0 tps, lat 614.530 ms stddev 95.902
> .
> progress: 5.0 s, 10.0 tps, lat 508.561 ms stddev 82.338
> client 3 script 0 aborted in command 1 query 0: ERROR:  could not find left 
> sibling of block 1451 in index "pgbench_accounts_pkey"
> progress: 6.0 s, 47.0 tps, lat 113.001 ms stddev 55.709
> .
> progress: 12.0 s, 84.0 tps, lat 48.451 ms stddev 7.238
> client 2 script 0 aborted in command 1 query 0: ERROR:  could not find left 
> sibling of block 2104 in index "pgbench_accounts_pkey"
> progress: 13.0 s, 87.0 tps, lat 39.338 ms stddev 5.417
> .
> progress: 16.0 s, 158.0 tps, lat 18.988 ms stddev 3.251
> client 4 script 0 aborted in command 1 query 0: ERROR:  could not find left 
> sibling of block 2501 in index "pgbench_accounts_pkey"

I was able to reproduce issue with vanilla PG_12 on different systems
including the Windows machine.
On some servers it happens 100% times. On others - very seldom.

It is possible to radically increase chance to reproduce the issue by
adding a sleep in btree_xlog_unlink_page[7]:
>   + pg_usleep(10 * 1000L);
>
>   /* Rewrite target page as empty deleted page */
>   buffer = XLogInitBufferForRedo(record, 0);

--  WHAT HAPPENS  --
It is race condition issue between btree_xlog_unlink_page and _bt_walk_left.

btree_xlog_unlink_page removes page from btree changing btpo_prev and
btpo_next of its left and right siblings to point
to the each other and marks target page as removed. All these
operations are done using one-page-at-a-time locking because of[4]:

>* In normal operation, we would lock all the pages this WAL record
>* touches before changing any of them.  In WAL replay, it should be okay
>* to lock just one page at a time, since no concurrent index updates can
>* be happening, and readers should not care whether they arrive at the
>* target page or not (since it's surely empty).

_bt_walk_left walks left in very tricky way. Please refer to
src/backend/access/nbtree/README for details[5]:

>Moving left in a backward scan is complicated because we must consider
>the possibility that the left sibling was just split (meaning we must find
>the rightmost page derived from the left sibling), plus the possibility
>that the page we were just on has now been deleted and hence isn't in the
>sibling chain at all anymore.

So, this is how race is happens:

0) this is target page (B) and its siblings.
A <---> B <---> C ---> END

1) walreceiver starts btree_xlog_unlink_page for the B. It is changes
the links from C to A and from A to C (I hope my scheme will be
displayed correctly):
A < B > C ---> END
^   ^
 \_/

But B is not marked as BTP_DELETED yet - walreceiver stops at nbtxlog:697[2].

2) other backend starts _bt_walk_left from B.
It checks A, goes to from A to C by updated btpo_next and later sees
end of the btree.
So, next step is to check if B was deleted (nbtsearch:2011)[3] and try
to recover.

But B is not yet deleted! It will be marked as BTP_DELETED after a few
millis by walreceiver but not yet.
So, nbtsearch:2046[6] is happens.


--  HOW TO FIX  --
The first idea was to mark page as BTP_DELETED before updating siblings lin

Re: error context for vacuum to include block number

[Alvaro Herrera]

On 2020-Mar-16, Amit Kapila wrote:

> 2.
> + /* Setup error traceback support for ereport() */
> + update_vacuum_error_cbarg(vacrelstats, VACUUM_ERRCB_PHASE_SCAN_HEAP,
> + InvalidBlockNumber, NULL);
> + errcallback.callback = vacuum_error_callback;
> + errcallback.arg = vacrelstats;
> + errcallback.previous = error_context_stack;
> + error_context_stack = &errcallback;
> ..
> ..
> + /* Init vacrelstats for use as error callback by parallel worker: */
> + vacrelstats.relnamespace = get_namespace_name(RelationGetNamespace(onerel));
> + vacrelstats.relname = pstrdup(RelationGetRelationName(onerel));
> + vacrelstats.indname = NULL;
> + vacrelstats.phase = VACUUM_ERRCB_PHASE_UNKNOWN; /* Not yet processing */
> +
> + /* Setup error traceback support for ereport() */
> + errcallback.callback = vacuum_error_callback;
> + errcallback.arg = &vacrelstats;
> + errcallback.previous = error_context_stack;
> + error_context_stack = &errcallback;
> +
> 
> I think the code can be bit simplified if we have a function
> setup_vacuum_error_ctx which takes necessary parameters and fill the
> required vacrelstats params, setup errcallback.  Then we can use
> update_vacuum_error_cbarg at required places.

Heh, he had that and I took it away -- it looked unnatural.  I thought
changing error_context_stack inside such a function, then resetting it
back to "previous" outside the function, was too leaky an abstraction.

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

Re: Option to dump foreign data in pg_dump

[David Steele]

Hi Luis,

Please don't top post. Also be careful to quote prior text when 
replying.  Your message was pretty hard to work through -- i.e. figuring 
out what you said vs. what you were replying to.


On 3/5/20 8:51 AM, Luis Carril wrote:


At this point I would like to leave the patch as is, and discuss further 
improvement in a future patch.


I have marked this as Need Review since the author wants the patch 
considered as-is.


I think Ashutosh, at least, has concerns about the patch as it stands, 
but does anyone else want to chime in?


Regards,
--
-David
da...@pgmasters.net

Re: ALTER tbl rewrite loses CLUSTER ON index

[Alvaro Herrera]

On 2020-Mar-16, Justin Pryzby wrote:

> Also, should we call it "is_index_clustered", since otherwise it sounds alot
> like "+get_index_clustered" (without "is"), which sounds like it takes a table
> and returns which index is clustered.  That might be just as useful for some 
> of
> these callers.

Amit's proposed name seems to match lsyscache.c usual conventions better.

> Should we use your get_index_isclustered more widely ?

Yeah, in cluster(), mark_index_clustered().

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

Re: Online checksums verification in the backend

[Julien Rouhaud]

On Mon, Mar 16, 2020 at 02:15:27PM +0100, Julien Rouhaud wrote:
> On Mon, Mar 16, 2020 at 09:42:39AM +0100, Julien Rouhaud wrote:
> > On Mon, Mar 16, 2020 at 01:53:35PM +0900, Masahiko Sawada wrote:
> > >
> > > In addition to comments from Michael-san, here are my comments:
>
> Thanks both for the reviews.  I'm attaching a v3 with all comments addressed,
> except:
>
> > It seems to me that this test would be a good fit for
> > src/test/modules/test_misc/.
>
>
> AFAICT this is explicitly documented as tests for various extensions, and for
> now it's a core function, so I didn't move it.
>
>
> > +Run
> > +make check
> > +or
> > +make installcheck
> > Why is installcheck mentioned here?
>
>
> This is actually already used in multiple other test readme.


Sorry I forgot to update the regression tests.  v4 attached.
>From e71af7998600a491efa1435d3c218de6e55bbc64 Mon Sep 17 00:00:00 2001
From: Julien Rouhaud 
Date: Mon, 4 Nov 2019 08:40:23 +0100
Subject: [PATCH v4] Add a pg_check_relation() function.

This functions checks the validity of the checksums for all non-dirty blocks of
a given relation, and optionally a given fork, and returns the list of all
blocks that don't match, along with the expected and found checksums.

Author: Julien Rouhaud
Reviewed-by: Michael Paquier, Masahiko Sawada
Discussion: 
https://postgr.es/m/CAOBaU_aVvMjQn%3Dge5qPiJOPMmOj5%3Dii3st5Q0Y%2BWuLML5sR17w%40mail.gmail.com
---
 doc/src/sgml/func.sgml|  44 ++
 src/backend/catalog/system_views.sql  |   7 +
 src/backend/storage/page/checksum.c   | 427 +-
 src/backend/utils/adt/Makefile|   1 +
 src/backend/utils/adt/checksumfuncs.c |  96 
 src/backend/utils/init/globals.c  |   8 +
 src/backend/utils/misc/guc.c  |  43 ++
 src/include/catalog/pg_proc.dat   |   8 +
 src/include/miscadmin.h   |   8 +
 src/include/storage/checksum.h|   7 +
 src/include/utils/guc_tables.h|   1 +
 src/test/Makefile |   3 +-
 src/test/check_relation/.gitignore|   2 +
 src/test/check_relation/Makefile  |  23 +
 src/test/check_relation/README|  23 +
 .../check_relation/t/01_checksums_check.pl| 276 +++
 16 files changed, 973 insertions(+), 4 deletions(-)
 create mode 100644 src/backend/utils/adt/checksumfuncs.c
 create mode 100644 src/test/check_relation/.gitignore
 create mode 100644 src/test/check_relation/Makefile
 create mode 100644 src/test/check_relation/README
 create mode 100644 src/test/check_relation/t/01_checksums_check.pl

diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 323366feb6..5847d7f655 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -21815,6 +21815,50 @@ SELECT (pg_stat_file('filename')).modification;
 
   
 
+  
+   Data Sanity Functions
+
+   
+The functions shown in 
+provide means to check for health of data file in a cluster.
+   
+
+   
+Data Sanity Functions
+
+ 
+  Name Return Type 
Description
+  
+ 
+
+ 
+  
+   
+pg_check_relation(relation 
oid, fork 
text)
+   
+   setof record
+   Validate the checksums for all blocks of a given relation, and
+   optionally the given fork.
+  
+ 
+
+   
+
+   
+pg_check_relation
+   
+   
+pg_check_relation iterates over all the blocks of all
+or the specified fork of a given relation and verify their checksum.  It
+returns the list of blocks for which the found checksum doesn't match the
+expected one.  You must be a member of the
+pg_read_all_stats role to use this function.  It can
+only be used if data checksums are enabled.  See  for more information.
+   
+
+  
+
   
 
   
diff --git a/src/backend/catalog/system_views.sql 
b/src/backend/catalog/system_views.sql
index b8a3f46912..e266292b03 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -1386,6 +1386,13 @@ LANGUAGE INTERNAL
 STRICT STABLE PARALLEL SAFE
 AS 'jsonb_path_query_first_tz';
 
+CREATE OR REPLACE FUNCTION pg_check_relation(
+  IN relation regclass DEFAULT NULL::regclass, IN fork text DEFAULT NULL::text,
+  OUT relid oid, OUT forknum integer, OUT failed_blocknum bigint,
+  OUT expected_checksum integer, OUT found_checksum integer)
+  RETURNS SETOF record VOLATILE LANGUAGE internal AS 'pg_check_relation'
+  PARALLEL RESTRICTED;
+
 --
 -- 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/storage/page/checksum.c 
b/src/backend/storage/page/checksum.c
index e010691c9f..aec5e2ad2d 100644
--- a/src/backend/storage/page/checksum.c
+++ b/src/backend/storage/page/checksum.c
@@ -15,8 +15,429 @@
 
 #include "storage/checksum.h"
 /*
- * The actual code is in storage/checksum_impl.

Re: [PATCH] Use PKG_CHECK_MODULES to detect the libxml2 library

[Daniel Gustafsson]

> On 13 Mar 2020, at 17:14, Tom Lane  wrote:
> Daniel Gustafsson  writes:

>> I read this is as a preventative patch to stay ahead of future changes to
>> packaging.  If these changes do materialize, won't they be equally likely to
>> hit installations for backbranch minors as v13?
> 
> Yeah, that's the argument *for* back-patching.  Question is whether it
> outweighs the risk of silently breaking somebody's build by linking
> to the wrong libxml2 version.

Correct, my argument is that breakage can be expected equally across branches,
so I think back-patching should be seriously considered.

>> We refer to both libxml and libxml2 in these paragraphs.  Since upstream is
>> consistently referring to it as libxml2, maybe we should take this as
>> opportunity to switch to that for the docs?
> 
> I think we're kind of stuck with "--with-libxml".  Conceivably we
> could introduce "--with-libxml2", redefine the old switch as an
> obsolete alias, and start saying "libxml2" instead of "libxml".
> But I'm not sure that's worth the trouble, and it seems like
> material for a different patch anyway.

Absolutely, thats why I referred to changing mentions of libxml in the docs
only where we refer to the product and not the switch (the latter was not very
clear in my email though).  Also, shouldn't libxml2 be within 
tags like OpenSSL and LLVM et.al?

cheers ./daniel

Re: Refactor compile-time assertion checks for C/C++

[Tom Lane]

Michael Paquier  writes:
> On Fri, Mar 13, 2020 at 11:00:33AM -0400, Tom Lane wrote:
>> If we do need to change it, I'd be inclined to just use the do{}
>> block everywhere, not bothering with the extra #if test.

> Not sure what you mean here because we cannot use the do{} flavor
> either for the C fallback, no?  See for example the definitions of
> unconstify() in c.h.

Sorry for being unclear --- I just meant that we could use do{}
in StaticAssertStmt for both C and C++.  Although now I notice
that the code is trying to use StaticAssertStmt for StaticAssertExpr,
which you're right isn't going to do.  But I think something like
this would work and be a bit simpler than what you proposed:

 #else
 /* Fallback implementation for C and C++ */
 #define StaticAssertStmt(condition, errmessage) \
-   ((void) sizeof(struct { int static_assert_failure : (condition) ? 1 : 
-1; }))
+   do { struct static_assert_struct { int static_assert_failure : 
(condition) ? 1 : -1; }; } while(0)
 #define StaticAssertExpr(condition, errmessage) \
-   StaticAssertStmt(condition, errmessage)
+   ((void) sizeof(struct { int static_assert_failure : (condition) ? 1 : 
-1; }))
 #define StaticAssertDecl(condition, errmessage) \


regards, tom lane

Re: add types to index storage params on doc

[Alvaro Herrera]

On 2020-Mar-16, Atsushi Torikoshi wrote:

> Thanks for your comments!
> 
> On Mon, Mar 16, 2020 at 11:49 AM Fujii Masao 
> wrote:
> 
> > -buffering
> > +buffering (string)
> >
> > Isn't it better to use "enum" rather than "string"?
> > In the docs about enum GUC parameters, "enum" is used there.
> 
> Agreed. I've fixed it to "enum".
> 
> But I'm now wondering about the type of check_option[3], [4].
> Because I decide the type to "string" referring to check_option, which is
> the other element of  enumRelOpts in reloptions.c.
> 
> Should I also change it to "enum"?

Yeah, these were strings until recently (commit 773df883e8f7 Sept 2019).

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

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

[Julien Rouhaud]

On Sat, Mar 14, 2020 at 06:53:51PM +0100, Julien Rouhaud wrote:
> On Tue, Mar 03, 2020 at 04:24:59PM +0100, Julien Rouhaud wrote:
> >
> > cfbot reports a failure since 2f9661311b (command completion tag
> > change), so here's a rebased v6, no change otherwise.
>
>
> Conflict with 8e8a0becb3 (Unify several ways to tracking backend type), thanks
> again to cfbot, rebased v7 attached.


Bit repetita.
>From 87be2c545e32c0c08a410949d5c5d383a4162af3 Mon Sep 17 00:00:00 2001
From: Julien Rouhaud 
Date: Mon, 18 Mar 2019 18:55:50 +0100
Subject: [PATCH v8 1/2] Expose queryid in pg_stat_activity and log_line_prefix

Similarly to other fields in pg_stat_activity, only the queryid from the top
level statements are exposed, and if the backends status isn't active then the
queryid from the last executed statements is displayed.

Also add a %Q placeholder to include the queryid in the log_line_prefix, which
will also only expose top level statements.

Author: Julien Rouhaud
Reviewed-by: Evgeny Efimkin, Michael Paquier
Discussion: https://postgr.es/m/CA+8PKvQnMfOE-c3YLRwxOsCYXQDyP8VXs6CDtMZp1V4=d4l...@mail.gmail.com
---
 .../pg_stat_statements/pg_stat_statements.c   | 179 --
 doc/src/sgml/config.sgml  |   9 +-
 doc/src/sgml/monitoring.sgml  |  12 ++
 src/backend/catalog/system_views.sql  |   1 +
 src/backend/executor/execMain.c   |   8 +
 src/backend/executor/execParallel.c   |  14 +-
 src/backend/executor/nodeGather.c |   3 +-
 src/backend/executor/nodeGatherMerge.c|   4 +-
 src/backend/parser/analyze.c  |   5 +
 src/backend/postmaster/pgstat.c   |  65 +++
 src/backend/tcop/postgres.c   |   5 +
 src/backend/utils/adt/pgstatfuncs.c   |   7 +-
 src/backend/utils/error/elog.c|  10 +-
 src/backend/utils/misc/postgresql.conf.sample |   1 +
 src/include/catalog/pg_proc.dat   |   6 +-
 src/include/executor/execParallel.h   |   3 +-
 src/include/pgstat.h  |   5 +
 src/test/regress/expected/rules.out   |   9 +-
 18 files changed, 267 insertions(+), 79 deletions(-)

diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index 20dc8c605b..2b3aa79cb6 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -112,6 +112,14 @@ static const uint32 PGSS_PG_MAJOR_VERSION = PG_VERSION_NUM / 100;
 
 #define JUMBLE_SIZE1024	/* query serialization buffer size */
 
+/*
+ * Utility statements that pgss_ProcessUtility and pgss_post_parse_analyze
+ * ignores.
+ */
+#define PGSS_HANDLED_UTILITY(n)		(!IsA(n, ExecuteStmt) && \
+	!IsA(n, PrepareStmt) && \
+	!IsA(n, DeallocateStmt))
+
 /*
  * Extension version number, for supporting older extension versions' objects
  */
@@ -308,7 +316,8 @@ static void pgss_ProcessUtility(PlannedStmt *pstmt, const char *queryString,
 ProcessUtilityContext context, ParamListInfo params,
 QueryEnvironment *queryEnv,
 DestReceiver *dest, QueryCompletion *qc);
-static uint64 pgss_hash_string(const char *str, int len);
+static const char *pgss_clean_querytext(const char *query, int *location, int *len);
+static uint64 pgss_compute_utility_queryid(const char *query, int query_len);
 static void pgss_store(const char *query, uint64 queryId,
 	   int query_location, int query_len,
 	   double total_time, uint64 rows,
@@ -792,16 +801,34 @@ pgss_post_parse_analyze(ParseState *pstate, Query *query)
 		return;
 
 	/*
-	 * Utility statements get queryId zero.  We do this even in cases where
-	 * the statement contains an optimizable statement for which a queryId
-	 * could be derived (such as EXPLAIN or DECLARE CURSOR).  For such cases,
-	 * runtime control will first go through ProcessUtility and then the
-	 * executor, and we don't want the executor hooks to do anything, since we
-	 * are already measuring the statement's costs at the utility level.
+	 * We compute a queryId now so that it can get exported in out
+	 * PgBackendStatus.  pgss_ProcessUtility will later discard it to prevents
+	 * double counting of optimizable statements that are directly contained in
+	 * utility statements.  Note that we don't compute a queryId for prepared
+	 * statemets related utility, as those will inherit from the underlying
+	 * statements's one (except DEALLOCATE which is entirely untracked).
 	 */
 	if (query->utilityStmt)
 	{
-		query->queryId = UINT64CONST(0);
+		if (pgss_track_utility && PGSS_HANDLED_UTILITY(query->utilityStmt)
+			&& pstate->p_sourcetext)
+		{
+			const char *querytext = pstate->p_sourcetext;
+			int query_location = query->stmt_location;
+			int query_len = query->stmt_len;
+
+			/*
+			 * Confine our attention to the relevant part of the string, if the
+			 * query is a portion of a multi-statement source string.
+			 */
+			queryt

Re: [PATCH] Connection time for \conninfo

[Juan José Santamaría Flecha]

On Mon, Mar 16, 2020 at 1:24 PM David Steele  wrote:

> On 3/14/20 3:16 PM, Rodrigo Ramírez Norambuena wrote:
> >
> > I forgot about that ... It passed a little time from my new pushed
> > changes. So go ahead :)
>
> This patch has been returned with feedback. Please feel free to resubmit
> in a future CF when you believe the feedback has been addressed.


There is a duplicate entry as:

https://commitfest.postgresql.org/27/2423/

Regards,

Juan José Santamaría Flecha

Re: [PATCH] Opclass parameters

[Alexander Korotkov]

Hi!

I took a look on this patchset.  There is a first set of questions.

* Patchset badly needs comments.  I've to literally reverse engineer
to get what's going on.  But I still don't understand many things.

* I'm curious about what local_relopts.base field means.

void
extend_local_reloptions(local_relopts *opts, void *base, Size base_size)
{
Assert(opts->base_size < base_size);
opts->base = base;
opts->base_size = base_size;
}

/*
 * add_local_reloption
 *  Add an already-created custom reloption to the local list.
 */
static void
add_local_reloption(local_relopts *relopts, relopt_gen *newoption, void *pval)
{
local_relopt *opt = palloc(sizeof(*opt));

opt->option = newoption;
opt->offset = (char *) pval - (char *) relopts->base;

relopts->options = lappend(relopts->options, opt);
}

Datum
ghstore_options(PG_FUNCTION_ARGS)
{
local_relopts *relopts = (local_relopts *) PG_GETARG_POINTER(0);
GistHstoreOptions *options = NULL;

extend_local_reloptions(relopts, options, sizeof(*options));
add_local_int_reloption(relopts, "siglen",
"signature length in bytes",
SIGLEN_DEFAULT, 1, SIGLEN_MAX,
&options->siglen);

PG_RETURN_VOID();
}

It's not commented, but I guess it's used to calculate offsets from
pointers passed to add_local_*_reloption().  Is it better to just pass
offsets to add_local_*_reloption()?

* It's generally unclear how does amattoptions and opclass options
interact.  As I get we now don't have an example where both
amattoptions and opclass options involved.  What is general benefit
from putting both two kind of options into single bytea?  Can opclass
options method do something useful with amattoptions?  For instance,
some amattoptions can be calculated from opclass options?  That would
be some point for putting these options together, but it doesn't look
like opclass options method can do this?

* It current opclass code safe for introduction new atattoptions.  For
instace, would ghstore_*() work the same way expecting
GistHstoreOptions struct to be passed as opclass options if gist would
introduce own attoptions?  I guess not.  If I'm wrong, please clarify
this.  And patchset needs comment one could get this without guessing.

--
Alexander Korotkov

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

Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*)

[Fabien COELHO]

About v11, ISTM that the recursive function should check for symbolic 
links and possibly avoid them:


 sh> cd data/base
 sh> ln -s .. foo

 psql> SELECT * FROM pg_ls_dir_recurse('.');
 ERROR:  could not stat file 
"./base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo":
 Too many levels of symbolic links
 CONTEXT:  SQL function "pg_ls_dir_recurse" statement 1

This probably means using lstat instead of (in supplement to?) stat, and 
probably tell if something is a link, and if so not recurse in them.


--
Fabien.

Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*)

[Justin Pryzby]

On Mon, Mar 16, 2020 at 04:20:21PM +0100, Fabien COELHO wrote:
> 
> About v11, ISTM that the recursive function should check for symbolic links
> and possibly avoid them:
> 
>  sh> cd data/base
>  sh> ln -s .. foo
> 
>  psql> SELECT * FROM pg_ls_dir_recurse('.');
>  ERROR:  could not stat file 
> "./base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo":
>  Too many levels of symbolic links
>  CONTEXT:  SQL function "pg_ls_dir_recurse" statement 1
> 
> This probably means using lstat instead of (in supplement to?) stat, and
> probably tell if something is a link, and if so not recurse in them.

Thanks for looking.

I think that opens up a can of worms.  I don't want to go into the business of
re-implementing all of find(1) - I count ~128 flags (most of which take
arguments).  You're referring to find -L vs find -P, and some people would want
one and some would want another.  And don't forget about find -H...

pg_stat_file doesn't expose the file type (I guess because it's not portable?),
and I think it's outside the scope of this patch to change that.  Maybe it
suggests that the pg_ls_dir_recurse patch should be excluded.

ISTM if someone wants to recursively list a directory, they should avoid
putting cycles there, or permission errors, or similar.  Or they should write
their own C extension that borrows from pg_ls_dir_files but handles more
arguments.

-- 
Justin

Re: pg11+: pg_ls_*dir LIMIT 1: temporary files .. not closed at end-of-transaction

[Justin Pryzby]

On Thu, Mar 12, 2020 at 07:11:56AM -0500, Justin Pryzby wrote:
> > Do you want to have a go at that?
> 
> First draft attached.  Note that I handled pg_ls_dir, even though I'm 
> proposing
> on the other thread to collapse/merge/meld it with pg_ls_dir_files [0].
> Possibly that's a bad idea with tuplestore, due to returning a scalar vs a row
> and needing to conditionally call CreateTemplateTupleDesc vs
> get_call_result_type.  I'll rebase that patch later today.
> 
> I didn't write test cases yet.  Also didn't look for functions not on your
> list.
> 
> I noticed this doesn't actually do anything, but kept it for now...except in
> pg_ls_dir error case:
> 
> src/include/utils/tuplestore.h:/* tuplestore_donestoring() used to be 
> required, but is no longer used */
> src/include/utils/tuplestore.h:#define tuplestore_donestoring(state)
> ((void) 0)

v2 attached - I will add to next CF in case you want to defer it until later.

-- 
Justin
>From 9ae75f693adf246ad551e258198606fd63190a20 Mon Sep 17 00:00:00 2001
From: Justin Pryzby 
Date: Wed, 11 Mar 2020 10:09:18 -0500
Subject: [PATCH v2 1/2] SRF: avoid leaking resources if not run to completion

Change to return a tuplestore populated immediately and returned in full.

Discussion: https://www.postgresql.org/message-id/20200308173103.GC1357%40telsasoft.com

See also: 9cb7db3f0 and 085b6b66
---
 contrib/adminpack/adminpack.c|  82 +-
 contrib/pgrowlocks/pgrowlocks.c  | 162 +--
 doc/src/sgml/xfunc.sgml  |  17 +-
 src/backend/utils/adt/datetime.c |  99 +---
 src/backend/utils/adt/genfile.c  | 111 +++--
 src/backend/utils/adt/misc.c | 116 +++--
 src/include/funcapi.h|   7 +-
 src/test/regress/expected/misc_functions.out |  18 +++
 src/test/regress/sql/misc_functions.sql  |   6 +
 9 files changed, 325 insertions(+), 293 deletions(-)

diff --git a/contrib/adminpack/adminpack.c b/contrib/adminpack/adminpack.c
index bc45e79895..070025f5a8 100644
--- a/contrib/adminpack/adminpack.c
+++ b/contrib/adminpack/adminpack.c
@@ -504,50 +504,56 @@ pg_logdir_ls_v1_1(PG_FUNCTION_ARGS)
 static Datum
 pg_logdir_ls_internal(FunctionCallInfo fcinfo)
 {
-	FuncCallContext *funcctx;
 	struct dirent *de;
-	directory_fctx *fctx;
+
+	ReturnSetInfo	*rsinfo = (ReturnSetInfo *) fcinfo->resultinfo;
+	MemoryContext	oldcontext;
+	TupleDesc		tupdesc;
+	Tuplestorestate	*tupstore;
+	bool			randomAccess;
+	DIR*dirdesc;
+
+	/* check to see if caller supports us returning a tuplestore */
+	if (rsinfo == NULL || !IsA(rsinfo, ReturnSetInfo))
+		ereport(ERROR,
+(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("set-valued function called in context that cannot accept a set")));
+	if (!(rsinfo->allowedModes & SFRM_Materialize))
+		ereport(ERROR,
+(errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("materialize mode required, but it is not allowed in this context")));
 
 	if (strcmp(Log_filename, "postgresql-%Y-%m-%d_%H%M%S.log") != 0)
 		ereport(ERROR,
 (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
  errmsg("the log_filename parameter must equal 'postgresql-%%Y-%%m-%%d_%%H%%M%%S.log'")));
 
-	if (SRF_IS_FIRSTCALL())
-	{
-		MemoryContext oldcontext;
-		TupleDesc	tupdesc;
-
-		funcctx = SRF_FIRSTCALL_INIT();
-		oldcontext = MemoryContextSwitchTo(funcctx->multi_call_memory_ctx);
+	/* The Tuplestore and TupleDesc should be created in ecxt_per_query_memory */
+	oldcontext = MemoryContextSwitchTo(rsinfo->econtext->ecxt_per_query_memory);
+	randomAccess = (rsinfo->allowedModes&SFRM_Materialize_Random) != 0;
+	tupstore = tuplestore_begin_heap(randomAccess, false, work_mem);
 
-		fctx = palloc(sizeof(directory_fctx));
+	/* Is this right ?? */
+	tupdesc = CreateTemplateTupleDesc(2);
+	TupleDescInitEntry(tupdesc, (AttrNumber) 1, "starttime",
+			TIMESTAMPOID, -1, 0);
+	TupleDescInitEntry(tupdesc, (AttrNumber) 2, "filename",
+			TEXTOID, -1, 0);
 
-		tupdesc = CreateTemplateTupleDesc(2);
-		TupleDescInitEntry(tupdesc, (AttrNumber) 1, "starttime",
-		   TIMESTAMPOID, -1, 0);
-		TupleDescInitEntry(tupdesc, (AttrNumber) 2, "filename",
-		   TEXTOID, -1, 0);
+	rsinfo->returnMode = SFRM_Materialize;
+	rsinfo->setResult = tupstore;
+	rsinfo->setDesc = tupdesc;
 
-		funcctx->attinmeta = TupleDescGetAttInMetadata(tupdesc);
+	MemoryContextSwitchTo(oldcontext);
 
-		fctx->location = pstrdup(Log_directory);
-		fctx->dirdesc = AllocateDir(fctx->location);
-
-		if (!fctx->dirdesc)
-			ereport(ERROR,
-	(errcode_for_file_access(),
-	 errmsg("could not open directory \"%s\": %m",
-			fctx->location)));
-
-		funcctx->user_fctx = fctx;
-		MemoryContextSwitchTo(oldcontext);
-	}
-
-	funcctx = SRF_PERCALL_SETUP();
-	fctx = (directory_fctx *) funcctx->user_fctx;
+	dirdesc = AllocateDir(Log_directory);
+	if (!dirdesc)
+		ereport(ERROR,
+(errcode_for_file_access(),
+ errmsg("could not open directory \"%s\": %m",
+		Log_directory)));

Re: [PATCH] Connection time for \conninfo

[David Steele]

On 3/16/20 10:59 AM, Juan José Santamaría Flecha wrote:
On Mon, Mar 16, 2020 at 1:24 PM David Steele > wrote:


On 3/14/20 3:16 PM, Rodrigo Ramírez Norambuena wrote:
 >
 > I forgot about that ... It passed a little time from my new pushed
 > changes. So go ahead :)

This patch has been returned with feedback. Please feel free to
resubmit
in a future CF when you believe the feedback has been addressed.


There is a duplicate entry as:


Thanks.  I've marked in as withdrawn but in the future you can feel free 
to do so yourself.


Regards,
--
-David
da...@pgmasters.net

JDBC prepared insert and X00 and SQL_ASCII

[gmail Vladimir Koković]

Hi,

I don't know if this is a bug or the intended mode,
but since ODBC works and JDBC does not, I would ask why JDBC prepared 
insert does not work if ODBC prepared insert works

in case some varchar field contains 0x00 and DB is SQL_ASCII?

My environment:
[root @ vlada-home ~ 16:32:56] $ psql -h localhost -U vlada 
asoftdev-ispp-pprostor-beograd

psql (13devel)
Type "help" for help.
asoftdev-ispp-pprostor-beograd = # \ l asoftdev-ispp-pprostor-beograd
 List of databases
  Name | Owner | Encoding | Collate | Ctype | Access privileges
 + --- + - - + - 
+ - + ---
 asoftdev-ispp-pprostor-belgrade | vlada | SQL_ASCII | en_US.UTF-8 | 
en_US.UTF-8 |

(1 row)
asoftdev-ispp-pprostor-beograd = # select version ();
version
-- 
--
 PostgreSQL 13devel on x86_64-pc-linux-gnu, compiled by gcc (GCC) 
9.2.0, 64-bit

(1 row)
asoftdev-ispp-pprostor-beograd = # \ q
[root @ vlada-home ~ 4:34:16 PM] $

cat /root/pglog/postgresql-2020-03-16_00.log
---
2020-03-16 16:04:18 CET [unknown] 127.0.0.1 (39582) 36708 0 0 LOG: 
connection received: host = 127.0.0.1 port = 39582
2020-03-16 16:04:18 CET asoftdev-ispp-pprostor-beograd 127.0.0.1 (39582) 
36708 0 0 LOG: connection authorized: user = vlada database = 
asoftdev-ispp-pprostor-beograd
2020-03-16 16:04:18 CET asoftdev-ispp-pprostor-beograd 127.0.0.1 (39582) 
36708 0 0 LOG: duration: 0.283 ms
2020-03-16 16:04:18 CET asoftdev-ispp-pprostor-beograd 127.0.0.1 (39582) 
36708 0 0 LOG: duration: 0.017 ms
2020-03-16 16:04:18 CET asoftdev-ispp-pprostor-beograd 127.0.0.1 (39582) 
36708 0 0 LOG: execute : SET extra_float_digits = 3
2020-03-16 16:04:18 CET asoftdev-ispp-pprostor-beograd 127.0.0.1 (39582) 
36708 0 0 LOG: duration: 0.046 ms
2020-03-16 16:04:18 CET asoftdev-ispp-pprostor-beograd 127.0.0.1 (39582) 
36708 0 0 LOG: duration: 0.029 ms
2020-03-16 16:04:18 CET asoftdev-ispp-pprostor-beograd 127.0.0.1 (39582) 
36708 0 0 LOG: duration: 0.010 ms
2020-03-16 16:04:18 CET asoftdev-ispp-pprostor-beograd 127.0.0.1 (39582) 
36708 0 0 LOG: execute : SET application_name = 'PostgreSQL 
JDBC Driver'
2020-03-16 16:04:18 CET asoftdev-ispp-pprostor-beograd 127.0.0.1 (39582) 
36708 0 0 LOG: duration: 0.043 ms
2020-03-16 16:04:18 CET asoftdev-ispp-pprostor-beograd 127.0.0.1 (39582) 
36708 0 0 LOG: duration: 0.094 ms
2020-03-16 16:04:18 CET asoftdev-ispp-pprostor-beograd 127.0.0.1 (39582) 
36708 0 0 LOG: duration: 0.019 ms
2020-03-16 16:04:18 CET asoftdev-ispp-pprostor-beograd 127.0.0.1 (39582) 
36708 0 0 LOG: execute : BEGIN
2020-03-16 16:04:18 CET asoftdev-ispp-pprostor-beograd 127.0.0.1 (39582) 
36708 0 0 LOG: duration: 0.201 ms
2020-03-16 16:04:18 CET asoftdev-ispp-pprostor-beograd 127.0.0.1 (39582) 
36708 0 0 LOG: duration: 0.041 ms
2020-03-16 16:04:18 CET asoftdev-ispp-pprostor-beograd 127.0.0.1 (39582) 
36708 0 0 LOG: duration: 0.023 ms
2020-03-16 16:04:18 CET asoftdev-ispp-pprostor-beograd 127.0.0.1 (39582) 
36708 0 0 LOG: execute : SET CLIENT_ENCODING TO SQL_ASCII
2020-03-16 16:04:18 CET asoftdev-ispp-pprostor-beograd 127.0.0.1 (39582) 
36708 0 0 LOG: duration: 0.061 ms
2020-03-16 16:04:18 CET asoftdev-ispp-pprostor-beograd 127.0.0.1 (39582) 
36708 0 0 LOG: duration: 2.372 ms
2020-03-16 16:04:18 CET asoftdev-ispp-pprostor-beograd 127.0.0.1 (39582) 
36708 0 0 LOG: duration: 1.788 ms
2020-03-16 16:04:18 CET asoftdev-ispp-pprostor-beograd 127.0.0.1 (39582) 
36708 0 0 LOG: execute : select keyfield, record from 
public.ispp_group order by keyfield

...
2020-03-16 16:04:41 CET asoftdev-ispp-pprostor-beograd 127.0.0.1 (39582) 
36708 137187 22021 ERROR: invalid byte sequence for encoding 
"SQL_ASCII": 0x00
2020-03-16 16:04:41 CET asoftdev-ispp-pprostor-belgrade 127.0.0.1 
(39582) 36708 137187 22021 STATEMENT: INSERT INTO 
Sekretariat_2019.ispp_promene VALUES ($1, $2, $3, $4, $5, $6, $7. $8, 
$9, $10, $11, $12, $13, $14, $15, $16, $17, $18, $19, $20, $21, $22, 
$23, $24, $25, $26, $27, $28, $29, $30, $31)
2020-03-16 16:05:15 CET asoftdev-ispp-pprostor-beograd 127.0.0.1 (39582) 
36708 0 0 LOG: disconnection: session time: 0: 00: 57.749 user = 
vlada database = asoftdev-ispp-pprostor- beograd host = 127.0.0.1 port = 
39582


Don't ask me why DB is SQL_ASCII!

Vladimir Kokovic, DP senior (69)
Serbia, Belgrade, March 16, 2020

Re: [PATCH] Use PKG_CHECK_MODULES to detect the libxml2 library

[Tom Lane]

Daniel Gustafsson  writes:
> On 13 Mar 2020, at 17:14, Tom Lane  wrote:
>> Yeah, that's the argument *for* back-patching.  Question is whether it
>> outweighs the risk of silently breaking somebody's build by linking
>> to the wrong libxml2 version.

> Correct, my argument is that breakage can be expected equally across branches,
> so I think back-patching should be seriously considered.

You're right that the risk of breakage (of either type) is about the same
across branches; but the project's conventions are not.  We try to avoid
unnecessary changes in back branches.

Still, after further reflection, I think the odds favor back-patching.
This patch could only break things on systems where

(a) There's more than one libxml2 installation, which is already a
tiny minority use-case.  It seems very unlikely to hurt any packagers
following typical build processes, for instance.

AND

(b) the default pkg-config and default xml2-config results differ.
That seems even more unlikely.

Now, breakage is certainly possible.  A counterexample to (b) is that
if you wanted to build using a non-default libxml2 installation, you
might've tried to select that by putting its xml2-config into your
PATH ahead of the system version, rather than setting XML2_CONFIG.
Post-patch, we'd consult pkg-config first and presumably end up
with the system libxml2.

Still, I think the number of people who'd get bit by that could be
counted without running out of fingers, while it seems quite likely
that many people will soon need to build our back branches on
platforms that won't have xml2-config.

So I'm now leaning to "back-patch and make sure to mention this in
the next release notes".  Barring objections, I'll do that soon.

> Absolutely, thats why I referred to changing mentions of libxml in the docs
> only where we refer to the product and not the switch (the latter was not very
> clear in my email though).  Also, shouldn't libxml2 be within 
> tags like OpenSSL and LLVM et.al?

I don't have a problem with s/libxml/libxml2/ in the running text
(without changing the switch name).  Can't get too excited about
 though.  I think we've only consistently applied
that tag to PostgreSQL itself.

regards, tom lane

Re: Re: A bug when use get_bit() function for a long bytea string

[Ashutosh Bapat]

On Fri, 13 Mar 2020 at 08:48, movead...@highgo.ca 
wrote:

> Thanks for the reply.
>
> >Why have you used size? Shouldn't we use int64?
> Yes, thanks for the point, I have changed the patch.
>
>

Thanks for the patch.


> >If get_bit()/set_bit() accept the second argument as int32, it can not
> >be used to set bits whose number does not fit 32 bits. I think we need
> >to change the type of the second argument as well.
> Because int32 can cover the length of bytea that PostgreSQL support,
>

I think the second argument indicates the bit position, which would be max
bytea length * 8. If max bytea length covers whole int32, the second
argument needs to be wider i.e. int64.

Some more comments on the patch
 struct pg_encoding
 {
- unsigned (*encode_len) (const char *data, unsigned dlen);
+ int64 (*encode_len) (const char *data, unsigned dlen);
  unsigned (*decode_len) (const char *data, unsigned dlen);
  unsigned (*encode) (const char *data, unsigned dlen, char *res);
  unsigned (*decode) (const char *data, unsigned dlen, char *res);

Why not use return type of int64 for rest of the functions here as well?

  res = enc->encode(VARDATA_ANY(data), datalen, VARDATA(result));

  /* Make this FATAL 'cause we've trodden on memory ... */
- if (res > resultlen)
+ if ((int64)res > resultlen)

if we change return type of all those functions to int64, we won't need
this cast.

Right now we are using int64 because bytea can be 1GB, but what if we
increase
that limit tomorrow, will int64 be sufficient? That may be unlikely in the
near
future, but nevertheless a possibility. Should we then define a new datatype
which resolves to int64 right now but really depends upon the bytea length.
I
am not suggesting that we have to do it right now, but may be something to
think about.

 hex_enc_len(const char *src, unsigned srclen)
 {
- return srclen << 1;
+ return (int64)(srclen << 1);

why not to convert srclen also to int64. That will also change the
pg_encoding
member definitions. But if encoded length requires int64 to fit the possibly
values, same would be true for the source lengths. Why can't the source
length
also be int64?

If still we want the cast, I think it should be ((int64)srclen << 1) rather
than casting the result.

  /* 3 bytes will be converted to 4, linefeed after 76 chars */
- return (srclen + 2) * 4 / 3 + srclen / (76 * 3 / 4);
+ return (int64)((srclen + 2) * 4 / 3 + srclen / (76 * 3 / 4));
similar comments as above.

 SELECT set_bit(B'0101011000100100', 16, 1); -- fail
 ERROR:  bit index 16 out of valid range (0..15)
+SELECT get_bit(
+   set_bit((repeat('Postgres', 512 * 1024 * 1024 / 8))::bytea, 0, 0)
+   ,0);
+ get_bit
+-
+   0
+(1 row)

It might help to add a test where we could pass the second argument
something
greater than 1G. But it may be difficult to write such a test case.

-- 
Best Wishes,
Ashutosh

Standby got fatal after the crash recovery

[Thunder]

Hello hackers:


Our standby node got fatal after the crash recovery. The fatal error was caused 
in slru module,  i changed log level from ERROR to PANIC and got the following 
stack.


(gdb) bt
#0  0x7f0cc47a1277 in raise () from /lib64/libc.so.6
#1  0x7f0cc47a2968 in abort () from /lib64/libc.so.6
#2  0x00a48347 in errfinish (dummy=dummy@entry=0) at elog.c:616
#3  0x005315dd in SlruReportIOError (ctl=ctl@entry=0xfbad00 
, pageno=1947, xid=xid@entry=63800060) at slru.c:1175
#4  0x00533152 in SimpleLruReadPage (ctl=ctl@entry=0xfbad00 
, pageno=1947, write_ok=write_ok@entry=true, 
xid=xid@entry=63800060) at slru.c:610
#5  0x00533350 in SimpleLruReadPage_ReadOnly (ctl=ctl@entry=0xfbad00 
, pageno=pageno@entry=1947, xid=xid@entry=63800060) at slru.c:680
#6  0x005293fd in TransactionIdGetStatus (xid=xid@entry=63800060, 
lsn=lsn@entry=0x7ffd17fc5130) at clog.c:661
#7  0x0053574a in TransactionLogFetch (transactionId=63800060) at 
transam.c:79
#8  TransactionIdDidCommit (transactionId=transactionId@entry=63800060) at 
transam.c:129
#9 0x004f1295 in HeapTupleHeaderAdvanceLatestRemovedXid 
(tuple=0x2aab27e936e0, latestRemovedXid=latestRemovedXid@entry=0x7ffd17fc51b0) 
at heapam.c:7672
#10 0x005103e0 in btree_xlog_delete_get_latestRemovedXid 
(record=record@entry=0x4636c98) at nbtxlog.c:656
#11 0x00510a19 in btree_xlog_delete (record=0x4636c98) at nbtxlog.c:707
#12 btree_redo (record=0x4636c98) at nbtxlog.c:1048
#13 0x005544a1 in StartupXLOG () at xlog.c:7825
#14 0x008185be in StartupProcessMain () at startup.c:226
#15 0x0058de15 in AuxiliaryProcessMain (argc=argc@entry=2, 
argv=argv@entry=0x7ffd17fc9430) at bootstrap.c:448
#16 0x00813fe4 in StartChildProcess (type=StartupProcess) at 
postmaster.c:5804
#17 0x00817eb0 in PostmasterMain (argc=argc@entry=3, 
argv=argv@entry=0x45ed6e0) at postmaster.c:1461
#18 0x004991f4 in main (argc=3, argv=0x45ed6e0) at main.c:232
(gdb) p /x *record
$10 = {wal_segment_size = 0x4000, read_page = 0x54f920, system_identifier = 
0x5e6e6ea4af938064, private_data = 0x7ffd17fc5390, ReadRecPtr = 0xe41e8fb28, 
EndRecPtr = 0xe41e8fbb0, decoded_record = 0x4634390, main_data = 0x4634c88, 
main_data_len = 0x54,
  main_data_bufsz = 0x1000, record_origin = 0x0, blocks = {{in_use = 0x1, rnode 
= {spcNode = 0x67f, dbNode = 0x7517968, relNode = 0x7517e81}, forknum = 0x0, 
blkno = 0x55f, flags = 0x0, has_image = 0x0, apply_image = 0x0, bkp_image = 
0x4632bc1, hole_offset = 0x40,
  hole_length = 0x1fb0, bimg_len = 0x50, bimg_info = 0x5, has_data = 0x0, 
data = 0x4656938, data_len = 0x0, data_bufsz = 0x2000}, {in_use = 0x0, rnode = 
{spcNode = 0x67f, dbNode = 0x7517968, relNode = 0x7517e2b}, forknum = 0x0, 
blkno = 0x3b5f, flags = 0x80,
  has_image = 0x0, apply_image = 0x0, bkp_image = 0x0, hole_offset = 0x0, 
hole_length = 0x0, bimg_len = 0x0, bimg_info = 0x0, has_data = 0x0, data = 
0x46468e8, data_len = 0x0, data_bufsz = 0x2000}, {in_use = 0x0, rnode = 
{spcNode = 0x67f, dbNode = 0x7517968,
relNode = 0x7e5c370}, forknum = 0x0, blkno = 0x2c3, flags = 0x80, 
has_image = 0x0, apply_image = 0x0, bkp_image = 0x0, hole_offset = 0x0, 
hole_length = 0x0, bimg_len = 0x0, bimg_info = 0x0, has_data = 0x0, data = 0x0, 
data_len = 0x0, data_bufsz = 0x0}, {
  in_use = 0x0, rnode = {spcNode = 0x67f, dbNode = 0x7517968, relNode = 
0x7517e77}, forknum = 0x0, blkno = 0xa8a, flags = 0x80, has_image = 0x0, 
apply_image = 0x0, bkp_image = 0x0, hole_offset = 0x0, hole_length = 0x0, 
bimg_len = 0x0, bimg_info = 0x0,
  has_data = 0x0, data = 0x0, data_len = 0x0, data_bufsz = 0x0}, {in_use = 
0x0, rnode = {spcNode = 0x0, dbNode = 0x0, relNode = 0x0}, forknum = 0x0, blkno 
= 0x0, flags = 0x0, has_image = 0x0, apply_image = 0x0, bkp_image = 0x0, 
hole_offset = 0x0, hole_length = 0x0,
  bimg_len = 0x0, bimg_info = 0x0, has_data = 0x0, data = 0x0, data_len = 
0x0, data_bufsz = 0x0} }, max_block_id = 0x0, readBuf = 
0x4632868, readLen = 0x2000, readSegNo = 0x39, readOff = 0x1e8e000, readPageTLI 
= 0x1, latestPagePtr = 0xe41e8e000,
  latestPageTLI = 0x1, currRecPtr = 0xe41e8fb28, currTLI = 0x0, 
currTLIValidUntil = 0x0, nextTLI = 0x0, readRecordBuf = 0x463, 
readRecordBufSize = 0xa000, errormsg_buf = 0x4634878, noPayload = 0x0, 
polar_logindex_meta_size = 0x2e}
(gdb) p /x minRecoveryPoint
$11 = 0xe41bbbfd8
(gdb) p reachedConsistency
$12 = true
(gdb) p standbyState
$13 = STANDBY_SNAPSHOT_READY
(gdb) p ArchiveRecoveryRequested
$14 = true


After the crash the standby redo started from 0xDBE1241D8, and reached 
consistency at 0xE41BBBFD8 because of previous minRecoveryPoint. It did not 
repaly all WAL record after the crash.
From the crash stack we see that it was reading clog to check xid= 63800060 
status.
But in wal file we see that xid= 63800060 was first created by xlog record 
which lsn=0xE42C22D68.


rmgr: Heaplen (rec/tot): 79/79, tx:   63800060, l

Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*)

[Fabien COELHO]

Hello Justin,


 psql> SELECT * FROM pg_ls_dir_recurse('.');
 ERROR:  could not stat file 
"./base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo":
 Too many levels of symbolic links
 CONTEXT:  SQL function "pg_ls_dir_recurse" statement 1

This probably means using lstat instead of (in supplement to?) stat, and
probably tell if something is a link, and if so not recurse in them.


Thanks for looking.

I think that opens up a can of worms.  I don't want to go into the business of
re-implementing all of find(1) - I count ~128 flags (most of which take
arguments).  You're referring to find -L vs find -P, and some people would want
one and some would want another.  And don't forget about find -H...


This is not the point. The point is that a link can change a finite tree 
into cyclic graph, and you do not want to delve into that, ever.


The "find" command, by default, does not recurse into a link because of 
said problem, and the user *must* ask for it and assume the infinite loop 
if any.


So if you implement one behavior, it should be not recursing into links. 
Franckly, I would not provide the recurse into link alternative, but it 
could be implemented if someone wants it, and the problem that come with 
it.



pg_stat_file doesn't expose the file type (I guess because it's not portable?),


You are right that Un*x and Windows are not the same wrt link. It seems 
that there is already something about that in port:


  "./src/port/dirmod.c:pgwin32_is_junction(const char *path)"

So most of the details are already hidden.


and I think it's outside the scope of this patch to change that.  Maybe it
suggests that the pg_ls_dir_recurse patch should be excluded.


IMHO, I really think that it should be included. Dealing with links is no 
big deal, but you need an additional column in _metadata to tell it is a 
link, and there is a ifdef because testing is a little different between 
unix and windows. I'd guess around 10-20 lines of code added.



ISTM if someone wants to recursively list a directory, they should avoid
putting cycles there, or permission errors, or similar.


Hmmm. I'd say the user should like to be able to call the function and 
never have a bad experience with it such as a failure on an infinite loop.


Or they should write their own C extension that borrows from 
pg_ls_dir_files but handles more arguments.


ISTM that the point of your patch is to provide the basic tool needed to 
list directories contents, and handling links somehow is a necessary part 
of that.


--
Fabien.

Make MemoryContextMemAllocated() more precise

[Jeff Davis]

AllocSet allocates memory for itself in blocks, which double in size up
to maxBlockSize. So, the current block (the last one malloc'd) may
represent half of the total memory allocated for the context itself.

The free space at the end of that block hasn't been touched at all, and
doesn't represent fragmentation or overhead. That means that the
"allocated" memory can be 2X the memory ever touched in the worst case.

Although that's technically correct, the purpose of
MemoryContextMemAllocated() is to give a "real" usage number so we know
when we're out of work_mem and need to spill (in particular, the disk-
based HashAgg work, but ideally other operators as well). This "real"
number should include fragmentation, freed-and-not-reused chunks, and
other overhead. But it should not include significant amounts of
allocated-but-never-touched memory, which says more about economizing
calls to malloc than it does about the algorithm's memory usage. 

Attached is a patch that makes mem_allocated a method (rather than a
field) of MemoryContext, and allows each memory context type to track
the memory its own way. They all do the same thing as before
(increment/decrement a field), but AllocSet also subtracts out the free
space in the current block. For Slab and Generation, we could do
something similar, but it's not as much of a problem because there's no
doubling of the allocation size.

Although I think this still matches the word "allocation" in spirit,
it's not technically correct, so feel free to suggest a new name for
MemoryContextMemAllocated().

Regards,
Jeff Davis

diff --git a/src/backend/utils/mmgr/aset.c b/src/backend/utils/mmgr/aset.c
index c0623f106d2..ccf78ffe0cb 100644
--- a/src/backend/utils/mmgr/aset.c
+++ b/src/backend/utils/mmgr/aset.c
@@ -132,6 +132,7 @@ typedef struct AllocSetContext
 	Size		maxBlockSize;	/* maximum block size */
 	Size		nextBlockSize;	/* next block size to allocate */
 	Size		allocChunkLimit;	/* effective chunk size limit */
+	Size		memAllocated;	/* track memory allocated for this context */
 	AllocBlock	keeper;			/* keep this block over resets */
 	/* freelist this context could be put in, or -1 if not a candidate: */
 	int			freeListIndex;	/* index in context_freelists[], or -1 */
@@ -272,6 +273,7 @@ static void *AllocSetRealloc(MemoryContext context, void *pointer, Size size);
 static void AllocSetReset(MemoryContext context);
 static void AllocSetDelete(MemoryContext context);
 static Size AllocSetGetChunkSpace(MemoryContext context, void *pointer);
+static Size AllocSetMemAllocated(MemoryContext context);
 static bool AllocSetIsEmpty(MemoryContext context);
 static void AllocSetStats(MemoryContext context,
 		  MemoryStatsPrintFunc printfunc, void *passthru,
@@ -291,6 +293,7 @@ static const MemoryContextMethods AllocSetMethods = {
 	AllocSetReset,
 	AllocSetDelete,
 	AllocSetGetChunkSpace,
+	AllocSetMemAllocated,
 	AllocSetIsEmpty,
 	AllocSetStats
 #ifdef MEMORY_CONTEXT_CHECKING
@@ -464,8 +467,7 @@ AllocSetContextCreateInternal(MemoryContext parent,
 parent,
 name);
 
-			((MemoryContext) set)->mem_allocated =
-set->keeper->endptr - ((char *) set);
+			set->memAllocated = set->keeper->endptr - ((char *) set);
 
 			return (MemoryContext) set;
 		}
@@ -555,7 +557,7 @@ AllocSetContextCreateInternal(MemoryContext parent,
 		parent,
 		name);
 
-	((MemoryContext) set)->mem_allocated = firstBlockSize;
+	set->memAllocated = firstBlockSize;
 
 	return (MemoryContext) set;
 }
@@ -617,7 +619,7 @@ AllocSetReset(MemoryContext context)
 		else
 		{
 			/* Normal case, release the block */
-			context->mem_allocated -= block->endptr - ((char*) block);
+			set->memAllocated -= block->endptr - ((char*) block);
 
 #ifdef CLOBBER_FREED_MEMORY
 			wipe_mem(block, block->freeptr - ((char *) block));
@@ -627,7 +629,7 @@ AllocSetReset(MemoryContext context)
 		block = next;
 	}
 
-	Assert(context->mem_allocated == keepersize);
+	Assert(set->memAllocated == keepersize);
 
 	/* Reset block size allocation sequence, too */
 	set->nextBlockSize = set->initBlockSize;
@@ -703,7 +705,7 @@ AllocSetDelete(MemoryContext context)
 		AllocBlock	next = block->next;
 
 		if (block != set->keeper)
-			context->mem_allocated -= block->endptr - ((char *) block);
+			set->memAllocated -= block->endptr - ((char *) block);
 
 #ifdef CLOBBER_FREED_MEMORY
 		wipe_mem(block, block->freeptr - ((char *) block));
@@ -715,7 +717,7 @@ AllocSetDelete(MemoryContext context)
 		block = next;
 	}
 
-	Assert(context->mem_allocated == keepersize);
+	Assert(set->memAllocated == keepersize);
 
 	/* Finally, free the context header, including the keeper block */
 	free(set);
@@ -758,7 +760,7 @@ AllocSetAlloc(MemoryContext context, Size size)
 		if (block == NULL)
 			return NULL;
 
-		context->mem_allocated += blksize;
+		set->memAllocated += blksize;
 
 		block->aset = set;
 		block->freeptr = block->endptr = ((char *) block) + blksize;
@@ -955,7 +957,7 @@ AllocSetAlloc(MemoryContext contex

Re: Internal key management system

[Bruce Momjian]

On Mon, Mar 16, 2020 at 04:13:21PM +0900, Masahiko Sawada wrote:
> On Thu, 12 Mar 2020 at 08:13, Bruce Momjian
>  wrote:
> >
> > On Fri, Mar  6, 2020 at 03:31:00PM +0900, Masahiko Sawada wrote:
> > > On Fri, 6 Mar 2020 at 15:25, Moon, Insung  
> > > wrote:
> > > >
> > > > Dear Sawada-san
> > > >
> > > > I don't know if my environment or email system is weird, but the V5
> > > > patch file is only include simply a changed list.
> > > > and previous V4 patch file size was 64kb, but the v5 patch file size 
> > > > was 2kb.
> > > > Can you check it?
> > > >
> > >
> > > Thank you! I'd attached wrong file.
> >
> > Looking at this thread, I wanted to make a few comments:
> >
> > Everyone seems to think pgcrypto need some maintenance.  Who would like
> > to take on that task?
> >
> > This feature does require openssl since all the encryption/decryption
> > happen via openssl function calls.
> >
> > Three are three levels of encrypt here:
> >
> > 1.  The master key generated during initdb
> >
> > 2.  The passphrase to unlock the master key at boot time.  Is that
> > optional or required?
> 
> The passphrase is required if the internal kms is enabled during
> initdb. Currently hashing the passphrase is also required but it could
> be optional. Even if we make hashing optional, we still require
> openssl to wrap and unwrap.

I think openssl should be required for any of this --- that is what I
was asking.

> > Could the wrap functions expose the master encryption key by passing in
> > empty string or null?
> 
> Currently the wrap function returns NULL if null is passed, and
> doesn't expose the master encryption key even if empty string is
> passed because we add random IV for each wrapping.

OK, good, makes sense, but you see why I am asking?  We never want the
master key to be visible.

> >  I wonder if we should create a derived key from
> > the master key to use for pg_wrap/pg_unwrap, maybe by appending a fixed
> > string to all strings supplied to these functions.  We could create
> > another derived key for use in block-level encryption, so we are sure
> > the two key spaces would never overlap.
> 
> Currently the master key is 32 bytes but you mean to add fixed string
> to the master key to derive a new key?

Yes, that was my idea --- make a separate keyspace for wrap/unwrap and
block-level encryption.

> > pgcryptokey shows a method for creating a secret between client and
> > server using SQL that does not expose the secret in the server logs:
> >
> > https://momjian.us/download/pgcryptokey/
> >
> > I assume we will create a 256-bit key for the master key, but give users
> > an option of  128-bit vs 256-bit keys for block-level encryption.
> > 256-bit keys are considered necessary for security against future
> > quantum computing attacks.
> 
> 256-bit keys are more weaker than 128-bit key in terms of quantum
> computing attacks?

No, I was saying we are using 256-bits for the master key and might
allow 128 or 256 keys for block encryption.

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

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +

Re: Berserk Autovacuum (let's save next Mandrill)

[Andres Freund]

Hi,

On 2020-03-13 19:10:00 -0500, Justin Pryzby wrote:
> On Fri, Mar 13, 2020 at 02:38:51PM -0700, Andres Freund wrote:
> > > |One disadvantage of decreasing vacuum_freeze_min_age is that it might 
> > > cause
> > > |VACUUM to do useless work: freezing a row version is a waste of time if 
> > > the row
> > > |is modified soon thereafter (causing it to acquire a new XID). So the 
> > > setting
> > > |should be large enough that rows are not frozen until they are unlikely 
> > > to
> > > |change any more.
> > 
> > I think the overhead here might be a bit overstated. Once a page is
> 
> Could you clarify if you mean the language in docs in general or specifically
> in the context of this patch ?

In the docs.

Greetings,

Andres Freund

Re: pgsql: Unify several ways to tracking backend type

[Alvaro Herrera]

On 2020-Mar-13, Peter Eisentraut wrote:

> Unify several ways to tracking backend type
> 
> Add a new global variable MyBackendType that uses the same BackendType
> enum that was previously only used by the stats collector.  That way
> several duplicate ways of checking what type a particular process is
> can be simplified.  Since it's no longer just for stats, move to
> miscinit.c and rename existing functions to match the expanded
> purpose.

Now that I look at this again, I realize that these backend-type
descriptions are not marked translatable, which is at odds with what we
do with HandlChildCrash, for example.

Now, in addition to plastering _() to the strings, maybe we could use
that new function in postmaster.c, say

HandleChildCrash(pid, exitstatus,
 GetBackendTypeDesc(B_CHECKPOINTER));

and so on.  Same with LogChildExit().  That'd reduce duplication.

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

Re: Planning counters in pg_stat_statements (using pgss_store)

[legrand legrand]

> I'm instead attaching a v7 which removes the assert in pg_plan_query, and
> modify pgss_planner_hook to also ignore queries without a query text, as
> this
> seems the best option. 

Ok, it was the second solution, go on !



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

Re: Berserk Autovacuum (let's save next Mandrill)

[Laurenz Albe]

On Mon, 2020-03-16 at 07:47 -0500, Justin Pryzby wrote:
> It seems to me that the easy thing to do is to implement this initially 
> without
> FREEZE (which is controlled by vacuum_freeze_table_age), and defer until
> July/v14 further discussion and implementation of another GUC/relopt for
> autovacuum freezing to be controlled by insert thresholds (or ratio).

Freezing tuples is the point of this patch.
As I have said, if you have a table where you insert many rows in few
transactions, you would trigger an autovacuum that then ends up doing nothing
because none of the rows have reached vacuum_freeze_table_age yet.

Then some time later you will get a really large vacuum run.

It seems to me that if we keep trying finding the formula that will vacuum
every table just right and never so the wrong thing, we will never get to 
anything.

Yours,
Laurenz Albe

Re: Berserk Autovacuum (let's save next Mandrill)

[Andres Freund]

Hi,

On 2020-03-16 20:49:43 +0100, Laurenz Albe wrote:
> On Mon, 2020-03-16 at 07:47 -0500, Justin Pryzby wrote:
> > It seems to me that the easy thing to do is to implement this initially 
> > without
> > FREEZE (which is controlled by vacuum_freeze_table_age), and defer until
> > July/v14 further discussion and implementation of another GUC/relopt for
> > autovacuum freezing to be controlled by insert thresholds (or ratio).
> 
> Freezing tuples is the point of this patch.

Sure. But not hurting existing installation is also a goal of the
patch. Since this is introducing potentially significant performance
downsides, I think it's good to be a bit conservative with the default
configuration.

I'm gettin a bit more bullish on implementing some of what what I
discussed in
https://www.postgresql.org/message-id/20200313213851.ejrk5gptnmp65uoo%40alap3.anarazel.de
at the same time as this patch.

In particularl, I think it'd make sense to *not* have a lower freezing
horizon for insert vacuums (because it *will* cause problems), but if
the page is dirty anyway, then do the freezing even if freeze_min_age
etc would otherwise prevent us from doing so?

It'd probably be ok to incur the WAL logging overhead unconditionally,
but I'm not sure about it.


> As I have said, if you have a table where you insert many rows in few
> transactions, you would trigger an autovacuum that then ends up doing nothing
> because none of the rows have reached vacuum_freeze_table_age yet.

> Then some time later you will get a really large vacuum run.

Well, only if you don't further insert into the table. Which isn't that
common a case for a table having a "really large vacuum run".


Greetings,

Andres Freund

Re: [PATCH] Use PKG_CHECK_MODULES to detect the libxml2 library

[Daniel Gustafsson]

> On 16 Mar 2020, at 17:12, Tom Lane  wrote:

> Still, I think the number of people who'd get bit by that could be
> counted without running out of fingers, while it seems quite likely
> that many people will soon need to build our back branches on
> platforms that won't have xml2-config.

I agree with this assessment.

> So I'm now leaning to "back-patch and make sure to mention this in
> the next release notes".  Barring objections, I'll do that soon.

None from me.

> I don't have a problem with s/libxml/libxml2/ in the running text
> (without changing the switch name).

+1

> Can't get too excited about
>  though.  I think we've only consistently applied
> that tag to PostgreSQL itself.

Fair enough.  Looking at a random sample it seems we use a bit of a mix of
nothing,  and  (the latter ones being more or
less equal in DocBook IIRC) to mark up externals.

cheers ./daniel

nbtree: Refactor "fastpath" and _bt_search() code

[Peter Geoghegan]

I have another refactoring patch for nbtinsert.c that is a little
bigger than the commits I've pushed recently. I thought I should run
it by -hackers before proceeding with commit, even though it seems
like a very clear improvement to me.

Attached patch creates a _bt_search() wrapper that is local to
nbtinsert.c -- _bt_search_insert(). This contains all the logic needed
to handle the "fastpath" rightmost leaf page caching optimization
added by commit 2b272734, avoiding any direct knowledge of the
optimization within the high level _bt_doinsert() function. This is
more or less how things were before 2b272734. This is certainly a
readability win.

It's also useful to have our leaf page buffer acquired within a
dedicated function that knows about the BTInsertState struct. That
makes the "ownership" of the buffer less ambiguous, and provides a
single reference point for other code that sets up the fastpath
optimization that will now actually be used in _bt_search_insert().

-- 
Peter Geoghegan


v1-0001-Refactor-_bt_doinsert-fastpath-optimization.patch
Description: Binary data

Re: Berserk Autovacuum (let's save next Mandrill)

[Justin Pryzby]

On Mon, Mar 16, 2020 at 08:49:43PM +0100, Laurenz Albe wrote:
> On Mon, 2020-03-16 at 07:47 -0500, Justin Pryzby wrote:
> > It seems to me that the easy thing to do is to implement this initially 
> > without
> > FREEZE (which is controlled by vacuum_freeze_table_age), and defer until
> > July/v14 further discussion and implementation of another GUC/relopt for
> > autovacuum freezing to be controlled by insert thresholds (or ratio).
> 
> Freezing tuples is the point of this patch.
> As I have said, if you have a table where you insert many rows in few
> transactions, you would trigger an autovacuum that then ends up doing nothing
> because none of the rows have reached vacuum_freeze_table_age yet.
> 
> Then some time later you will get a really large vacuum run.

Best practice is to vacuum following bulk load.  I don't think this patch is
going to change that.  Bulk-loaded tuples will be autovacuumed, which is nice,
but I don't think it'll be ideal if large bulk loads trigger an autovacuum with
cost delays which ISTM if it runs with FREEZE will take even longer.

If it's a bulk load, then I think it's okay to assume it was vacuumed, or
otherwise that it'll eventually be hit by autovac at some later date.

If it's not a "bulk load" but a normal runtime, and the table continues to
receive inserts/deletes, then eventually it'll hit a vacuum threshold and
tuples can be frozen.

If it receives a bunch of activity, which then stops (like a partition of a
table of timeseries data), then maybe it doesn't hit a vacuum threshold, until
wraparound vacuum.  I think in that case it's not catastrophic, since then it
wasn't big enough to hit any threshold (it's partitioned).  If every day,
autovacuum kicks in and does wraparound vacuum on table with data from (say)
100 days ago, I think that's reasonable.

One case which would suck is if the insert_threshold were 1e6, and you restore
a DB with 1000 tables of historic data (which are no longer being inserted
into) which have 9e5 rows each (just below the threshold).  Then autovacuum
will hit them all at once.  The solution to that is to manual vacuum after bulk
load, same as today.  As a practical matter, some of the tables are likely to
hit the autovacuum insert threshold, and some are likely to be pruned (or
updated) before wraparound vacuum, so the patch usually does improve that case.

-- 
Justin

Re: Just for fun: Postgres 20?

[Bruce Momjian]

On Wed, Feb 12, 2020 at 10:38:19PM +0100, Michael Banck wrote:
> Hi,
> 
> On Wed, Feb 12, 2020 at 02:52:53PM +0100, Andreas Karlsson wrote:
> > On 2/12/20 12:07 AM, Alvaro Herrera wrote:
> > > marcelo zen escribió:
> > > > I'd rather have releases being made when the software is ready and
> > > > not when the calendar year mandates it.  It seems like a terrible
> > > > idea.
> > > 
> > > But we do actually release on calendar year.  While it seems not
> > > unreasonable that we might fail to ship in time, that would likely lead
> > > to one month, two months of delay.  Four months?  I don't think anybody
> > > even imagines such a long delay.  It would be seen as utter,
> > > unacceptable failure of our release team.
> > 
> > It has actually happened once: PostgreSQL 9.5 was released in 2016-01-07.
> 
> It was my undestanding that this prompted us to form the release team,
> which has since done a great job of making sure that this does not
> happen again.

FYI, the delay for 9.5 was because the compression method used for JSONB
was discovered to be sub-optimal in August/September.  While a relesae
team might have gotten the release out before January, that isn't
certain.

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

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +

Re: Berserk Autovacuum (let's save next Mandrill)

[Laurenz Albe]

On Mon, 2020-03-16 at 13:13 -0700, Andres Freund wrote:
> > Freezing tuples is the point of this patch.
> 
> Sure. But not hurting existing installation is also a goal of the
> patch. Since this is introducing potentially significant performance
> downsides, I think it's good to be a bit conservative with the default
> configuration.
> 
> I'm gettin a bit more bullish on implementing some of what what I
> discussed in
> https://www.postgresql.org/message-id/20200313213851.ejrk5gptnmp65uoo%40alap3.anarazel.de
> at the same time as this patch.
>
> In particularl, I think it'd make sense to *not* have a lower freezing
> horizon for insert vacuums (because it *will* cause problems), but if
> the page is dirty anyway, then do the freezing even if freeze_min_age
> etc would otherwise prevent us from doing so?

I don't quite see why freezing tuples in insert-only tables will cause
problems - are you saying that more WAL will be written compared to
freezing with a higher freeze_min_age?

> > As I have said, if you have a table where you insert many rows in few
> > transactions, you would trigger an autovacuum that then ends up doing 
> > nothing
> > because none of the rows have reached vacuum_freeze_table_age yet.
> > Then some time later you will get a really large vacuum run.
> 
> Well, only if you don't further insert into the table. Which isn't that
> common a case for a table having a "really large vacuum run".

Ah, yes, you are right.
So it actually would not be worse if we use the normal freeze_min_age
for insert-only vacuums.

So do you think the patch would be ok as it is if we change only that?

Yours,
Laurenz Albe

Re: Berserk Autovacuum (let's save next Mandrill)

[Laurenz Albe]

On Mon, 2020-03-16 at 16:07 -0500, Justin Pryzby wrote:
> Best practice is to vacuum following bulk load.

Yes.

> If it's a bulk load, then I think it's okay to assume it was vacuumed,

No.  This patch is there precisely because too many people don't know
that they should vacuum their table after a bulk insert.
The idea of autovacuum is to do these things for you atomatically.

Yours,
Laurenz Albe

Re: Berserk Autovacuum (let's save next Mandrill)

[Andres Freund]

Hi,

On 2020-03-16 22:25:11 +0100, Laurenz Albe wrote:
> On Mon, 2020-03-16 at 13:13 -0700, Andres Freund wrote:
> > > Freezing tuples is the point of this patch.
> > 
> > Sure. But not hurting existing installation is also a goal of the
> > patch. Since this is introducing potentially significant performance
> > downsides, I think it's good to be a bit conservative with the default
> > configuration.
> > 
> > I'm gettin a bit more bullish on implementing some of what what I
> > discussed in
> > https://www.postgresql.org/message-id/20200313213851.ejrk5gptnmp65uoo%40alap3.anarazel.de
> > at the same time as this patch.
> >
> > In particularl, I think it'd make sense to *not* have a lower freezing
> > horizon for insert vacuums (because it *will* cause problems), but if
> > the page is dirty anyway, then do the freezing even if freeze_min_age
> > etc would otherwise prevent us from doing so?
> 
> I don't quite see why freezing tuples in insert-only tables will cause
> problems - are you saying that more WAL will be written compared to
> freezing with a higher freeze_min_age?

As far as I understand the patch may trigger additional vacuums e.g. for
tables that have some heavily updated parts / key ranges, and otherwise
are largely insert only (as long as there are in total considerably more
inserts than updates). That's not at all uncommon.

And for the heavily updated regions the additional vacuums with a 0 min
age could prove to be costly.  I've not looked at the new code, but it'd
be particularly bad if the changes were to trigger the
lazy_check_needs_freeze() check in lazy_scan_heap() - it'd have the
potential for a lot more contention.


> > > As I have said, if you have a table where you insert many rows in few
> > > transactions, you would trigger an autovacuum that then ends up doing 
> > > nothing
> > > because none of the rows have reached vacuum_freeze_table_age yet.
> > > Then some time later you will get a really large vacuum run.
> > 
> > Well, only if you don't further insert into the table. Which isn't that
> > common a case for a table having a "really large vacuum run".
> 
> Ah, yes, you are right.
> So it actually would not be worse if we use the normal freeze_min_age
> for insert-only vacuums.

Well, it's still be worse, because it'd likely trigger more writes of
the same pages. Once for setting hint bits during the first vacuum, and
then later a second for freezing. Which is why I was pondering using the
logic


> So do you think the patch would be ok as it is if we change only that?

I've not looked at it in enough detail so far to say either way, sorry.

Greetings,

Andres Freund

Re: Portal->commandTag as an enum

[Alvaro Herrera]

On 2020-Mar-04, Mark Dilger wrote:

> 
> 
> > On Mar 2, 2020, at 1:57 PM, Alvaro Herrera  wrote:
> > 
> > I pushed it now.
> 
> Thanks again!  While rebasing some other work on top, I noticed one of your 
> comments is out of date:
> 
> --- a/src/include/tcop/cmdtaglist.h
> +++ b/src/include/tcop/cmdtaglist.h
> @@ -23,7 +23,7 @@
>   * textual name, so that we can bsearch on it; see GetCommandTagEnum().
>   */
>  
> -/* symbol name, textual name, event_trigger_ok, table_rewrite_ok, rowcount, 
> last_oid */
> +/* symbol name, textual name, event_trigger_ok, table_rewrite_ok, rowcount */

Oops.  Pushed, thanks.

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

Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*)

[Justin Pryzby]

On Mon, Mar 16, 2020 at 04:20:21PM +0100, Fabien COELHO wrote:
> This probably means using lstat instead of (in supplement to?) stat, and
> probably tell if something is a link, and if so not recurse in them.

On Mon, Mar 16, 2020 at 07:21:06PM +0100, Fabien COELHO wrote:
> IMHO, I really think that it should be included. Dealing with links is no
> big deal, but you need an additional column in _metadata to tell it is a
> link

Instead of showing another column, I changed to show links with isdir=false.
At the cost of two more patches, to allow backpatching docs and maybe separate
commit to make the subtle change obvious in commit history, at least.

I see a few places in the backend and a few more in the fronted using the same
logic that I used for islink(), but I'm not sure if there's a good place to put
that to allow factoring out at least the other backend ones.

-- 
Justin
>From 1bb8e0efb4f14fa344cd5ee66c3138184a9fa9e2 Mon Sep 17 00:00:00 2001
From: Justin Pryzby 
Date: Fri, 6 Mar 2020 16:50:07 -0600
Subject: [PATCH v12 01/11] Document historic behavior about hiding directories
 and special files

Should backpatch to v10: tmpdir, waldir and archive_statusdir
---
 doc/src/sgml/func.sgml | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 323366feb6..4c0ea5ab3f 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -21450,6 +21450,7 @@ postgres=# SELECT * FROM pg_walfile_name_offset(pg_stop_backup());
 (mtime) of each file in the log directory. By default, only superusers
 and members of the pg_monitor role can use this function.
 Access may be granted to others using GRANT.
+Filenames beginning with a dot, directories, and other special files are not shown.

 

@@ -21461,6 +21462,7 @@ postgres=# SELECT * FROM pg_walfile_name_offset(pg_stop_backup());
 default only superusers and members of the pg_monitor role
 can use this function. Access may be granted to others using
 GRANT.
+Filenames beginning with a dot, directories, and other special files are not shown.

 

@@ -21473,6 +21475,7 @@ postgres=# SELECT * FROM pg_walfile_name_offset(pg_stop_backup());
 superusers and members of the pg_monitor role can
 use this function. Access may be granted to others using
 GRANT.
+Filenames beginning with a dot, directories, and other special files are not shown.

 

-- 
2.17.0

>From b6eb6cd9299e8ae07770790f05da038fede5278c Mon Sep 17 00:00:00 2001
From: Justin Pryzby 
Date: Mon, 16 Mar 2020 14:12:55 -0500
Subject: [PATCH v12 02/11] Document historic behavior of links to
 directories..

Backpatch to 9.5: pg_stat_file
---
 doc/src/sgml/func.sgml | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 4c0ea5ab3f..ace95fe661 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -21527,7 +21527,8 @@ SELECT convert_from(pg_read_binary_file('file_in_utf8.txt'), 'UTF8');
 size, last accessed time stamp, last modified time stamp,
 last file status change time stamp (Unix platforms only),
 file creation time stamp (Windows only), and a boolean
-indicating if it is a directory.  Typical usages include:
+indicating if it is a directory (or a symbolic link to a directory).
+Typical usages include:
 
 SELECT * FROM pg_stat_file('filename');
 SELECT (pg_stat_file('filename')).modification;
-- 
2.17.0

>From 52f5b76cc278c43983b044b7fb0f04c2d848d61e Mon Sep 17 00:00:00 2001
From: Justin Pryzby 
Date: Fri, 6 Mar 2020 17:12:04 -0600
Subject: [PATCH v12 03/11] Document historic behavior about hiding directories
 and special files

Should backpatch to v12: tmpdir
---
 doc/src/sgml/func.sgml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index ace95fe661..2c6142a0e0 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -21489,6 +21489,7 @@ postgres=# SELECT * FROM pg_walfile_name_offset(pg_stop_backup());
 default only superusers and members of the pg_monitor
 role can use this function.  Access may be granted to others using
 GRANT.
+Filenames beginning with a dot, directories, and other special files are not shown.

 

-- 
2.17.0

>From dd98e2b4f1fb37065a317719f0dd493e8837c7e7 Mon Sep 17 00:00:00 2001
From: Justin Pryzby 
Date: Mon, 9 Mar 2020 22:40:24 -0500
Subject: [PATCH v12 04/11] Add tests exercizing pg_ls_tmpdir..

..and its backing function pg_ls_dir_files
---
 src/test/regress/expected/misc_functions.out | 7 +++
 src/test/regress/input/tablespace.source | 5 +
 src/test/regress/output/tablespace.source| 8 
 src/test/regress/sql/misc_functions.sql  | 4 
 4 files changed, 24 insertions(+)

diff --git a/src/test/regress/expected/misc_functions.out b/src/test/regress/expected/misc_functions.out
index e217b678d7..9947d9ef9d 100644
--- a/src/test/regress/expected/mi

Re: Planning counters in pg_stat_statements (using pgss_store)

[Julien Rouhaud]

On Mon, Mar 16, 2020 at 01:34:11AM +, imai.yoshik...@fujitsu.com wrote:
> On Sat, Mar 14, 2020 at 5:28 PM, Julien Rouhaud wrote:
> > I don't think that's a correct assumption.  I obviously didn't read all of
> > citus extension, but it looks like what's happening is that they get 
> > generate a
> > custom Query from the original one, with all the modification needed for
> > distributed execution and whatnot, which is then fed to the planner.  I 
> > think
> > it's entirely mossible that the modified Query herits from a previously set
> > queryid, while still not really having a query text.  And if citus doesn't 
> > do
> > that, it doesn't seem like an illegal use cuse anyway.
>
> Indeed. It can happen that queryid has some value while query_text is NULL.
>
>
> > I'm instead attaching a v7 which removes the assert in pg_plan_query, and
> > modify pgss_planner_hook to also ignore queries without a query text, as 
> > this
> > seems the best option.
>
> Thank you.
> It also seems to me that is the best option.


Thanks Imai-san and PAscal for the feedback, it seems that we have an
agreement!


> BTW, I recheck the patchset.
> I think codes are ready for committer but should we modify the documentation?
> {min,max,mean,stddev}_time is now obsoleted so it is better to modify it to
> {min,max,mean,stddev}_exec_time and add {min,max,mean,stddev}_plan_time.


Oh indeed, I totally forgot about this.  I'm attaching v8 with updated
documentation that should match what was implemented since some versions.
>From 16c888f7861300d08d21f3ebb97af4fad3ec1fa5 Mon Sep 17 00:00:00 2001
From: Julien Rouhaud 
Date: Thu, 28 Mar 2019 13:33:23 +0100
Subject: [PATCH v8 1/2] Pass query string to the planner

---
 src/backend/commands/copy.c  |  3 ++-
 src/backend/commands/createas.c  |  3 ++-
 src/backend/commands/explain.c   |  2 +-
 src/backend/commands/extension.c |  2 +-
 src/backend/commands/matview.c   |  2 +-
 src/backend/commands/portalcmds.c|  2 +-
 src/backend/executor/functions.c |  1 +
 src/backend/optimizer/plan/planner.c | 10 ++
 src/backend/tcop/postgres.c  | 13 -
 src/backend/utils/cache/plancache.c  |  3 ++-
 src/include/optimizer/optimizer.h|  3 ++-
 src/include/optimizer/planner.h  |  4 +++-
 src/include/tcop/tcopprot.h  |  6 --
 13 files changed, 34 insertions(+), 20 deletions(-)

diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index fbde9f88e7..efb1e0d03e 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -1580,7 +1580,8 @@ BeginCopy(ParseState *pstate,
 		}
 
 		/* plan the query */
-		plan = pg_plan_query(query, CURSOR_OPT_PARALLEL_OK, NULL);
+		plan = pg_plan_query(query, pstate->p_sourcetext,
+			 CURSOR_OPT_PARALLEL_OK, NULL);
 
 		/*
 		 * With row level security and a user using "COPY relation TO", we
diff --git a/src/backend/commands/createas.c b/src/backend/commands/createas.c
index 3a5676fb39..9febdc5165 100644
--- a/src/backend/commands/createas.c
+++ b/src/backend/commands/createas.c
@@ -329,7 +329,8 @@ ExecCreateTableAs(ParseState *pstate, CreateTableAsStmt *stmt,
 		Assert(query->commandType == CMD_SELECT);
 
 		/* plan the query */
-		plan = pg_plan_query(query, CURSOR_OPT_PARALLEL_OK, params);
+		plan = pg_plan_query(query, pstate->p_sourcetext,
+			 CURSOR_OPT_PARALLEL_OK, params);
 
 		/*
 		 * Use a snapshot with an updated command ID to ensure this query sees
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index d901dc4a50..fb772aa5cd 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -375,7 +375,7 @@ ExplainOneQuery(Query *query, int cursorOptions,
 		INSTR_TIME_SET_CURRENT(planstart);
 
 		/* plan the query */
-		plan = pg_plan_query(query, cursorOptions, params);
+		plan = pg_plan_query(query, queryString, cursorOptions, params);
 
 		INSTR_TIME_SET_CURRENT(planduration);
 		INSTR_TIME_SUBTRACT(planduration, planstart);
diff --git a/src/backend/commands/extension.c b/src/backend/commands/extension.c
index 00cf4ef268..38cbea385a 100644
--- a/src/backend/commands/extension.c
+++ b/src/backend/commands/extension.c
@@ -751,7 +751,7 @@ execute_sql_string(const char *sql)
 		   NULL,
 		   0,
 		   NULL);
-		stmt_list = pg_plan_queries(stmt_list, CURSOR_OPT_PARALLEL_OK, NULL);
+		stmt_list = pg_plan_queries(stmt_list, sql, CURSOR_OPT_PARALLEL_OK, NULL);
 
 		foreach(lc2, stmt_list)
 		{
diff --git a/src/backend/commands/matview.c b/src/backend/commands/matview.c
index c3954f3e24..e5a5eef102 100644
--- a/src/backend/commands/matview.c
+++ b/src/backend/commands/matview.c
@@ -391,7 +391,7 @@ refresh_matview_datafill(DestReceiver *dest, Query *query,
 	CHECK_FOR_INTERRUPTS();
 
 	/* Plan the query which will generate data for the refresh. */
-	plan = pg_plan_query(query, 0, NULL);
+	plan = pg_plan_query(query, queryString, 0, NULL);
 
 	/*
 	 * Use a snapshot with an updated

Re: Add PostgreSQL home page to --help output

[Bruce Momjian]

On Fri, Feb 28, 2020 at 02:02:17PM +0100, Peter Eisentraut wrote:
> On 2020-02-20 12:09, Daniel Gustafsson wrote:
> > > On 20 Feb 2020, at 10:53, Daniel Gustafsson  wrote:
> > > 
> > > > On 20 Feb 2020, at 10:15, Peter Eisentraut 
> > > >  wrote:
> > > > 
> > > > On 2020-02-13 14:24, Greg Stark wrote:
> > > > > Sounds like a fine idea. But personally I would prefer it without the 
> > > > > <> around the it, just a url on a line by itself. I think it would be 
> > > > > clearer, look cleaner, and be easier to select to copy/paste 
> > > > > elsewhere.
> > > > 
> > > > I'm on the fence about this one, but I like the delimiters because it 
> > > > would also work consistently if we put a URL into running text where it 
> > > > might be immediately adjacent to other characters.  So I was actually 
> > > > going for easier to copy/paste here, but perhaps in other environments 
> > > > it's not easier?
> > > 
> > > For URLs completely on their own, not using <> makes sense.  Copy pasting 
> > > 
> > > into the location bar of Safari makes it load the url, but Firefox and 
> > > Chrome
> > > turn it into a search engine query (no idea about Windows browsers).
> > > 
> > > For URLs in running text it's not uncommon to have <> around the URL for 
> > > the
> > > very reason you mention.  Looking at --help and manpages from random open
> > > source tools there seems to be roughly a 50/50 split on using <> or not.
> > 
> > RFC3986 discuss this in , 
> > with
> > the content mostly carried over from RFC2396 appendix E.
> 
> I think we weren't going to get any more insights here, so I have committed
> it as is.

Some new feedback.  I find this output confusing since there is a colon
before the <>:

Report bugs to .
PostgreSQL home page: 

Does this look better (no colon)?

Report bugs to .
PostgreSQL home page 

or this (colon, no <>)?

Report bugs to .
PostgreSQL home page:  https://www.postgresql.org/

or maybe this?

Report bugs:  pgsql-b...@lists.postgresql.org
PostgreSQL home page:  https://www.postgresql.org/

or this?

Report bugs 
PostgreSQL home page 

I actually have never seen URLs in <>, only email addresses.  I think
using <> for URLs and emails is confusing because they usually have
different actions, unless we want to add mailto:

Report bugs 
PostgreSQL home page 

or

Report bugs mailto:pgsql-b...@lists.postgresql.org
PostgreSQL home page https://www.postgresql.org/

I kind of prefer the last one since the can both be pasted directly into
a browser.

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

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +

Re: Add PostgreSQL home page to --help output

[Bruce Momjian]

On Mon, Mar 16, 2020 at 05:55:26PM -0400, Bruce Momjian wrote:
>   Report bugs mailto:pgsql-b...@lists.postgresql.org
>   PostgreSQL home page https://www.postgresql.org/
> 
> I kind of prefer the last one since the can both be pasted directly into
> a browser.

Actually, I prefer:

Report bugs  mailto:pgsql-b...@lists.postgresql.org
PostgreSQL website  https://www.postgresql.org/

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

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +

Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*)

[Alvaro Herrera]

I pushed 0001 and 0003 (as a single commit).  archive_statusdir didn't
get here until 12, so your commit message was mistaken.  Also, pg10 is
slightly different so it didn't apply there, so I left it alone.

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

Adding test coverage for ALTER SEQUENCE .. SET SCHEMA

[Mark Dilger]

Hackers,

This is pretty low priority stuff, but I noticed there is no test coverage for 
this in src/test/regress.  There is underwhelming coverage elsewhere.  I leave 
it to others to decide if this is worth committing.



v1-0001-Adding-test-coverage-for-ALTER-SEQUENCE-SET-SCHEM.patch
Description: Binary data

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

Adding missing object access hook invocations

[Mark Dilger]

Hackers,

While working on object access hooks, I noticed several locations where I would 
expect the hook to be invoked, but no actual invocation.  I think this just 
barely qualifies as a bug.  It's debatable because whether it is a bug depends 
on the user's expectations and whether not invoking the hook in these cases is 
defensible.  Does anybody have any recollection of an intentional choice not to 
invoke in these locations?

Patch attached.



v1-0001-Adding-missing-Object-Access-hook-invocations.patch
Description: Binary data

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

RE: Planning counters in pg_stat_statements (using pgss_store)

[imai.yoshik...@fujitsu.com]

On Mon, Mar 16, 2020 at 9:49 PM, Julien Rouhaud wrote:
> On Mon, Mar 16, 2020 at 01:34:11AM +, imai.yoshik...@fujitsu.com
> wrote:
> > On Sat, Mar 14, 2020 at 5:28 PM, Julien Rouhaud wrote:
> > > I don't think that's a correct assumption.  I obviously didn't read
> > > all of citus extension, but it looks like what's happening is that
> > > they get generate a custom Query from the original one, with all the
> > > modification needed for distributed execution and whatnot, which is
> > > then fed to the planner.  I think it's entirely mossible that the
> > > modified Query herits from a previously set queryid, while still not
> > > really having a query text.  And if citus doesn't do that, it doesn't 
> > > seem like
> an illegal use cuse anyway.
> >
> > Indeed. It can happen that queryid has some value while query_text is NULL.
> >
> >
> > > I'm instead attaching a v7 which removes the assert in
> > > pg_plan_query, and modify pgss_planner_hook to also ignore queries
> > > without a query text, as this seems the best option.
> >
> > Thank you.
> > It also seems to me that is the best option.
> 
> 
> Thanks Imai-san and PAscal for the feedback, it seems that we have an
> agreement!
> 
> 
> > BTW, I recheck the patchset.
> > I think codes are ready for committer but should we modify the
> documentation?
> > {min,max,mean,stddev}_time is now obsoleted so it is better to modify
> > it to {min,max,mean,stddev}_exec_time and add
> {min,max,mean,stddev}_plan_time.
> 
> 
> Oh indeed, I totally forgot about this.  I'm attaching v8 with updated
> documentation that should match what was implemented since some
> versions.

Okay, I checked it.
So I'll mark this as a ready for committer.

Thanks
--
Yoshikazu Imai

Re: Add PostgreSQL home page to --help output

[Alvaro Herrera]

On 2020-Mar-16, Bruce Momjian wrote:

> On Mon, Mar 16, 2020 at 05:55:26PM -0400, Bruce Momjian wrote:
> > Report bugs mailto:pgsql-b...@lists.postgresql.org
> > PostgreSQL home page https://www.postgresql.org/
> > 
> > I kind of prefer the last one since the can both be pasted directly into
> > a browser.
> 
> Actually, I prefer:
> 
>   Report bugs  mailto:pgsql-b...@lists.postgresql.org
>   PostgreSQL website  https://www.postgresql.org/

Hmm, pasting mailto into the browser address bar doesn't work for me ...
it just goes to the lists.postgresql.org website (Brave) or sits there
doing nothing (Firefox).  I was excited there for a minute.

If we're talking personal preference, I like the current output.

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

Re: Adding missing object access hook invocations

[Alvaro Herrera]

On 2020-Mar-16, Mark Dilger wrote:

> Hackers,
> 
> While working on object access hooks, I noticed several locations where I 
> would expect the hook to be invoked, but no actual invocation.  I think this 
> just barely qualifies as a bug.  It's debatable because whether it is a bug 
> depends on the user's expectations and whether not invoking the hook in these 
> cases is defensible.  Does anybody have any recollection of an intentional 
> choice not to invoke in these locations?

Hmm, possibly the create-time calls are missing.

I'm surprised about the InvokeObjectDropHook calls though.  Doesn't
deleteOneObject already call that?  If we have more calls elsewhere,
maybe they are redundant.  I think we should only have those for
"shared" objects.

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

Re: Berserk Autovacuum (let's save next Mandrill)

[Laurenz Albe]

On Mon, 2020-03-16 at 14:34 -0700, Andres Freund wrote:
> > > In particularl, I think it'd make sense to *not* have a lower freezing
> > > horizon for insert vacuums (because it *will* cause problems), but if
> > > the page is dirty anyway, then do the freezing even if freeze_min_age
> > > etc would otherwise prevent us from doing so?
> > 
> > I don't quite see why freezing tuples in insert-only tables will cause
> > problems - are you saying that more WAL will be written compared to
> > freezing with a higher freeze_min_age?
> 
> As far as I understand the patch may trigger additional vacuums e.g. for
> tables that have some heavily updated parts / key ranges, and otherwise
> are largely insert only (as long as there are in total considerably more
> inserts than updates). That's not at all uncommon.
> 
> And for the heavily updated regions the additional vacuums with a 0 min
> age could prove to be costly.  I've not looked at the new code, but it'd
> be particularly bad if the changes were to trigger the
> lazy_check_needs_freeze() check in lazy_scan_heap() - it'd have the
> potential for a lot more contention.

I think I got it.

Here is a version of the patch that does *not* freeze more tuples than
normal, except if a prior tuple on the same page is already eligible for 
freezing.

lazy_check_needs_freeze() is only called for an aggressive vacuum, which
this isn't.

Does that look sane?

Yours,
Laurenz Albe
From abf3c092e016bbf19059fc104669e92a8de18462 Mon Sep 17 00:00:00 2001
From: Laurenz Albe 
Date: Tue, 17 Mar 2020 01:02:56 +0100
Subject: [PATCH] Autovacuum tables that have received only inserts

Add "autovacuum_vacuum_insert_threshold" and
"autovacuum_vacuum_insert_scale_factor" GUC and reloption.
The default value for the threshold is 1000.
The scale factor defaults to 0, which means that it is
effectively disabled, but it offers some flexibility
to tune the feature similar to other autovacuum knobs.

Any table that has received more inserts since it was
last vacuumed (and that is not vacuumed for another
reason) will be autovacuumed.
During such a vacuum run, freeze all tuples in a page
that has already been dirtied for any other reason.
This should cause little extra overhead.

This avoids the known problem that insert-only tables
are never autovacuumed until they need to have their
anti-wraparound autovacuum, which then can be massive
and disruptive.

To track the number of inserts since the last vacuum,
introduce a StatTabEntry "inserts_since_vacuum" that
gets reset to 0 after a vacuum.  This value is available
in "pg_stat_*_tables" as "n_ins_since_vacuum".

Author: Laurenz Albe, based on a suggestion from Darafei Praliaskouski
Reviewed-by: David Rowley, Justin Pryzby, Masahiko Sawada, Andres Freund
Discussion: https://postgr.es/m/CAC8Q8t+j36G_bLF=+0imo6jgnwnlnwb1tujxujr-+x8zcct...@mail.gmail.com
---
 doc/src/sgml/config.sgml  | 41 ++
 doc/src/sgml/maintenance.sgml |  9 
 doc/src/sgml/monitoring.sgml  |  5 ++
 doc/src/sgml/ref/create_table.sgml| 30 +++
 src/backend/access/common/reloptions.c| 22 
 src/backend/access/heap/vacuumlazy.c  | 11 +++-
 src/backend/catalog/system_views.sql  |  1 +
 src/backend/commands/vacuum.c |  3 ++
 src/backend/postmaster/autovacuum.c   | 53 ---
 src/backend/postmaster/pgstat.c   |  5 ++
 src/backend/utils/adt/pgstatfuncs.c   | 16 ++
 src/backend/utils/misc/guc.c  | 20 +++
 src/backend/utils/misc/postgresql.conf.sample |  4 ++
 src/bin/psql/tab-complete.c   |  4 ++
 src/include/catalog/pg_proc.dat   |  5 ++
 src/include/commands/vacuum.h |  1 +
 src/include/pgstat.h  |  1 +
 src/include/postmaster/autovacuum.h   |  2 +
 src/include/utils/rel.h   |  2 +
 src/test/regress/expected/rules.out   |  3 ++
 20 files changed, 230 insertions(+), 8 deletions(-)

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index c1128f89ec..0ed1bb9d5e 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -7244,6 +7244,26 @@ COPY postgres_log FROM '/full/path/to/logfile.csv' WITH csv;
   
  
 
+ 
+  autovacuum_vacuum_insert_threshold (integer)
+  
+   autovacuum_vacuum_insert_threshold
+   configuration parameter
+  
+  
+  
+   
+Specifies the number of inserted tuples needed to trigger a
+VACUUM in any one table.
+The default is 1000 tuples.
+This parameter can only be set in the postgresql.conf
+file or on the server command line;
+but the setting can be overridden for individual tables by
+changing table storage parameters.
+   
+  
+ 
+
  
   autovacuum_analyze_threshold (integer)
   
@@ -7285,6 +7305,27 @@ COPY postgr

Re: Parallel leader process info in EXPLAIN

[Thomas Munro]

On Tue, Mar 17, 2020 at 2:39 AM David Steele  wrote:
> On 1/26/20 7:03 PM, Thomas Munro wrote:
> > Fair point.  I will look into that.
>
> Are you still planning on looking at this patch for PG13?
>
> Based on the current state (002 abandoned, 001 needs total rework) I'd
> say it should just be Returned with Feedback or Closed for now.

When you put it like that, yeah :-)  I marked it returned with
feedback.  Thanks Melanie and Rafia for the reviews so far, and I'll
be back with a new version for PG14.

Re: Adding missing object access hook invocations

[Mark Dilger]

> On Mar 16, 2020, at 5:14 PM, Alvaro Herrera  wrote:
> 
> On 2020-Mar-16, Mark Dilger wrote:
> 
>> Hackers,
>> 
>> While working on object access hooks, I noticed several locations where I 
>> would expect the hook to be invoked, but no actual invocation.  I think this 
>> just barely qualifies as a bug.  It's debatable because whether it is a bug 
>> depends on the user's expectations and whether not invoking the hook in 
>> these cases is defensible.  Does anybody have any recollection of an 
>> intentional choice not to invoke in these locations?
> 
> Hmm, possibly the create-time calls are missing.

It looks to me that both the create and alter calls are missing.

> 
> I'm surprised about the InvokeObjectDropHook calls though.  Doesn't
> deleteOneObject already call that?  If we have more calls elsewhere,
> maybe they are redundant.  I think we should only have those for
> "shared" objects.

Yeah, you are right about the drop hook being invoked elsewhere for dropping 
ACCESS METHOD and STATISTICS.  Sorry for the noise.

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

Re: pg11+: pg_ls_*dir LIMIT 1: temporary files .. not closed at end-of-transaction

[Tom Lane]

Justin Pryzby  writes:
> v2 attached - I will add to next CF in case you want to defer it until later.

Thanks, reviewed and pushed.  Since this is a bug fix (at least in part)
I didn't want to wait.

regards, tom lane

Re: pg11+: pg_ls_*dir LIMIT 1: temporary files .. not closed at end-of-transaction

[Justin Pryzby]

On Mon, Mar 16, 2020 at 09:38:50PM -0400, Tom Lane wrote:
> Justin Pryzby  writes:
> > v2 attached - I will add to next CF in case you want to defer it until 
> > later.
> 
> Thanks, reviewed and pushed.  Since this is a bug fix (at least in part)
> I didn't want to wait.

Thanks for fixing my test case and pushing.

-- 
Justin

Re: Refactor compile-time assertion checks for C/C++

[Michael Paquier]

On Mon, Mar 16, 2020 at 10:32:36AM -0400, Tom Lane wrote:
> Sorry for being unclear --- I just meant that we could use do{}
> in StaticAssertStmt for both C and C++.  Although now I notice
> that the code is trying to use StaticAssertStmt for StaticAssertExpr,
> which you're right isn't going to do.  But I think something like
> this would work and be a bit simpler than what you proposed:
> 
>  #else
>  /* Fallback implementation for C and C++ */
>  #define StaticAssertStmt(condition, errmessage) \
> - ((void) sizeof(struct { int static_assert_failure : (condition) ? 1 : 
> -1; }))
> + do { struct static_assert_struct { int static_assert_failure : 
> (condition) ? 1 : -1; }; } while(0)
>  #define StaticAssertExpr(condition, errmessage) \
> - StaticAssertStmt(condition, errmessage)
> + ((void) sizeof(struct { int static_assert_failure : (condition) ? 1 : 
> -1; }))
>  #define StaticAssertDecl(condition, errmessage) \

C++ does not allow defining a struct inside a sizeof() call, so in
this case StaticAssertExpr() does not work with the previous extension
in C++.  StaticAssertStmt() does the work though.

One alternatine I can think of for C++ would be something like the
following, though C does not like this flavor either:
typedef char static_assert_struct[condition ? 1 : -1]
--
Michael


signature.asc
Description: PGP signature

Re: Add PostgreSQL home page to --help output

[Michael Paquier]

On Mon, Mar 16, 2020 at 09:10:25PM -0300, Alvaro Herrera wrote:
> On 2020-Mar-16, Bruce Momjian wrote:
>> Actually, I prefer:
>> 
>>  Report bugs  mailto:pgsql-b...@lists.postgresql.org
>>  PostgreSQL website  https://www.postgresql.org/
> 
> Hmm, pasting mailto into the browser address bar doesn't work for me ...
> it just goes to the lists.postgresql.org website (Brave) or sits there
> doing nothing (Firefox).  I was excited there for a minute.

Pasting "mailto:pgsql-b...@lists.postgresql.org"; to Firefox 74.0 pops
up for me a window asking to choose an application able to send an
email.  For example, with mutt, this would begin generating an email
sent to the address pasted.

> If we're talking personal preference, I like the current output.

No strong opinion about one or the other.
--
Michael


signature.asc
Description: PGP signature

Re: Refactor compile-time assertion checks for C/C++

[Tom Lane]

Michael Paquier  writes:
> On Mon, Mar 16, 2020 at 10:32:36AM -0400, Tom Lane wrote:
>> Sorry for being unclear --- I just meant that we could use do{}
>> in StaticAssertStmt for both C and C++.  Although now I notice
>> that the code is trying to use StaticAssertStmt for StaticAssertExpr,
>> which you're right isn't going to do.  But I think something like
>> this would work and be a bit simpler than what you proposed:
>> 
>> #else
>> /* Fallback implementation for C and C++ */
>> #define StaticAssertStmt(condition, errmessage) \
>> -((void) sizeof(struct { int static_assert_failure : (condition) ? 1 : 
>> -1; }))
>> +do { struct static_assert_struct { int static_assert_failure : 
>> (condition) ? 1 : -1; }; } while(0)
>> #define StaticAssertExpr(condition, errmessage) \
>> -StaticAssertStmt(condition, errmessage)
>> +((void) sizeof(struct { int static_assert_failure : (condition) ? 1 : 
>> -1; }))
>> #define StaticAssertDecl(condition, errmessage) \

> C++ does not allow defining a struct inside a sizeof() call, so in
> this case StaticAssertExpr() does not work with the previous extension
> in C++.  StaticAssertStmt() does the work though.

[ scratches head... ]  A do{} is okay in an expression in C++ ??

regards, tom lane

Re:Standby got fatal after the crash recovery

[Thunder]

Appreciate any suggestion for this issue?
Is there something i misunderstand?
















At 2020-03-17 00:33:36, "Thunder"  wrote:

Hello hackers:


Our standby node got fatal after the crash recovery. The fatal error was caused 
in slru module,  i changed log level from ERROR to PANIC and got the following 
stack.


(gdb) bt
#0  0x7f0cc47a1277 in raise () from /lib64/libc.so.6
#1  0x7f0cc47a2968 in abort () from /lib64/libc.so.6
#2  0x00a48347 in errfinish (dummy=dummy@entry=0) at elog.c:616
#3  0x005315dd in SlruReportIOError (ctl=ctl@entry=0xfbad00 
, pageno=1947, xid=xid@entry=63800060) at slru.c:1175
#4  0x00533152 in SimpleLruReadPage (ctl=ctl@entry=0xfbad00 
, pageno=1947, write_ok=write_ok@entry=true, 
xid=xid@entry=63800060) at slru.c:610
#5  0x00533350 in SimpleLruReadPage_ReadOnly (ctl=ctl@entry=0xfbad00 
, pageno=pageno@entry=1947, xid=xid@entry=63800060) at slru.c:680
#6  0x005293fd in TransactionIdGetStatus (xid=xid@entry=63800060, 
lsn=lsn@entry=0x7ffd17fc5130) at clog.c:661
#7  0x0053574a in TransactionLogFetch (transactionId=63800060) at 
transam.c:79
#8  TransactionIdDidCommit (transactionId=transactionId@entry=63800060) at 
transam.c:129
#9 0x004f1295 in HeapTupleHeaderAdvanceLatestRemovedXid 
(tuple=0x2aab27e936e0, latestRemovedXid=latestRemovedXid@entry=0x7ffd17fc51b0) 
at heapam.c:7672
#10 0x005103e0 in btree_xlog_delete_get_latestRemovedXid 
(record=record@entry=0x4636c98) at nbtxlog.c:656
#11 0x00510a19 in btree_xlog_delete (record=0x4636c98) at nbtxlog.c:707
#12 btree_redo (record=0x4636c98) at nbtxlog.c:1048
#13 0x005544a1 in StartupXLOG () at xlog.c:7825
#14 0x008185be in StartupProcessMain () at startup.c:226
#15 0x0058de15 in AuxiliaryProcessMain (argc=argc@entry=2, 
argv=argv@entry=0x7ffd17fc9430) at bootstrap.c:448
#16 0x00813fe4 in StartChildProcess (type=StartupProcess) at 
postmaster.c:5804
#17 0x00817eb0 in PostmasterMain (argc=argc@entry=3, 
argv=argv@entry=0x45ed6e0) at postmaster.c:1461
#18 0x004991f4 in main (argc=3, argv=0x45ed6e0) at main.c:232
(gdb) p /x *record
$10 = {wal_segment_size = 0x4000, read_page = 0x54f920, system_identifier = 
0x5e6e6ea4af938064, private_data = 0x7ffd17fc5390, ReadRecPtr = 0xe41e8fb28, 
EndRecPtr = 0xe41e8fbb0, decoded_record = 0x4634390, main_data = 0x4634c88, 
main_data_len = 0x54,
  main_data_bufsz = 0x1000, record_origin = 0x0, blocks = {{in_use = 0x1, rnode 
= {spcNode = 0x67f, dbNode = 0x7517968, relNode = 0x7517e81}, forknum = 0x0, 
blkno = 0x55f, flags = 0x0, has_image = 0x0, apply_image = 0x0, bkp_image = 
0x4632bc1, hole_offset = 0x40,
  hole_length = 0x1fb0, bimg_len = 0x50, bimg_info = 0x5, has_data = 0x0, 
data = 0x4656938, data_len = 0x0, data_bufsz = 0x2000}, {in_use = 0x0, rnode = 
{spcNode = 0x67f, dbNode = 0x7517968, relNode = 0x7517e2b}, forknum = 0x0, 
blkno = 0x3b5f, flags = 0x80,
  has_image = 0x0, apply_image = 0x0, bkp_image = 0x0, hole_offset = 0x0, 
hole_length = 0x0, bimg_len = 0x0, bimg_info = 0x0, has_data = 0x0, data = 
0x46468e8, data_len = 0x0, data_bufsz = 0x2000}, {in_use = 0x0, rnode = 
{spcNode = 0x67f, dbNode = 0x7517968,
relNode = 0x7e5c370}, forknum = 0x0, blkno = 0x2c3, flags = 0x80, 
has_image = 0x0, apply_image = 0x0, bkp_image = 0x0, hole_offset = 0x0, 
hole_length = 0x0, bimg_len = 0x0, bimg_info = 0x0, has_data = 0x0, data = 0x0, 
data_len = 0x0, data_bufsz = 0x0}, {
  in_use = 0x0, rnode = {spcNode = 0x67f, dbNode = 0x7517968, relNode = 
0x7517e77}, forknum = 0x0, blkno = 0xa8a, flags = 0x80, has_image = 0x0, 
apply_image = 0x0, bkp_image = 0x0, hole_offset = 0x0, hole_length = 0x0, 
bimg_len = 0x0, bimg_info = 0x0,
  has_data = 0x0, data = 0x0, data_len = 0x0, data_bufsz = 0x0}, {in_use = 
0x0, rnode = {spcNode = 0x0, dbNode = 0x0, relNode = 0x0}, forknum = 0x0, blkno 
= 0x0, flags = 0x0, has_image = 0x0, apply_image = 0x0, bkp_image = 0x0, 
hole_offset = 0x0, hole_length = 0x0,
  bimg_len = 0x0, bimg_info = 0x0, has_data = 0x0, data = 0x0, data_len = 
0x0, data_bufsz = 0x0} }, max_block_id = 0x0, readBuf = 
0x4632868, readLen = 0x2000, readSegNo = 0x39, readOff = 0x1e8e000, readPageTLI 
= 0x1, latestPagePtr = 0xe41e8e000,
  latestPageTLI = 0x1, currRecPtr = 0xe41e8fb28, currTLI = 0x0, 
currTLIValidUntil = 0x0, nextTLI = 0x0, readRecordBuf = 0x463, 
readRecordBufSize = 0xa000, errormsg_buf = 0x4634878, noPayload = 0x0, 
polar_logindex_meta_size = 0x2e}
(gdb) p /x minRecoveryPoint
$11 = 0xe41bbbfd8
(gdb) p reachedConsistency
$12 = true
(gdb) p standbyState
$13 = STANDBY_SNAPSHOT_READY
(gdb) p ArchiveRecoveryRequested
$14 = true


After the crash the standby redo started from 0xDBE1241D8, and reached 
consistency at 0xE41BBBFD8 because of previous minRecoveryPoint. It did not 
repaly all WAL record after the crash.
From the crash stack we see that it was reading clog to check xid= 63800060 
status.
But in wal file we see that x

Re: RecoveryWalAll and RecoveryWalStream wait events

[Fujii Masao]

On 2020/03/15 0:06, Atsushi Torikoshi wrote:

On 2020/02/19 21:46 Fujii Masao mailto:masao.fu...@oss.nttdata.com>>:
 >> I agree to the former, I think RecoveryWalInterval works well enough.
 >RecoveryWalInterval sounds confusing to me...

IMHO as a user, I prefer RecoveryRetrieveRetryInterval because
it's easy to understand this wait_event is related to the
parameter 'wal_retrieve_retry_interval'.

Also from the point of balance, the explanation of
RecoveryRetrieveRetryInterval is lengthy, but I
sometimes feel explanations of wait_events in the
manual are so simple that it's hard to understand
well.


+1 to document them more. It's not easy task, though..


 >    Waiting when WAL data is not available from any kind of sources
 >    (local, archive or stream) before trying again to retrieve WAL data,

I think 'local' means pg_wal here, but the comment on
WaitForWALToBecomeAvailable() says checking pg_wal in
standby mode is 'not documented', so I'm a little bit worried
that users may be confused.


This logic seems to be documented in high-availability.sgml.
But, anyway, you think that "pg_wal" should be used instead of "local" here?

Regards,

--
Fujii Masao
NTT DATA CORPORATION
Advanced Platform Technology Group
Research and Development Headquarters

Re:Re:Standby got fatal after the crash recovery

[Thunder]

The slru error detail is the following.
DETAIL:  Could not read from file "/data/pg_xact/003C" at offset 221184: 
Success.


I think read /data/pg_xact/003C from offset 221184 and the return value was 0.













At 2020-03-17 10:36:03, "Thunder"  wrote:

Appreciate any suggestion for this issue?
Is there something i misunderstand?
















At 2020-03-17 00:33:36, "Thunder"  wrote:

Hello hackers:


Our standby node got fatal after the crash recovery. The fatal error was caused 
in slru module,  i changed log level from ERROR to PANIC and got the following 
stack.


(gdb) bt
#0  0x7f0cc47a1277 in raise () from /lib64/libc.so.6
#1  0x7f0cc47a2968 in abort () from /lib64/libc.so.6
#2  0x00a48347 in errfinish (dummy=dummy@entry=0) at elog.c:616
#3  0x005315dd in SlruReportIOError (ctl=ctl@entry=0xfbad00 
, pageno=1947, xid=xid@entry=63800060) at slru.c:1175
#4  0x00533152 in SimpleLruReadPage (ctl=ctl@entry=0xfbad00 
, pageno=1947, write_ok=write_ok@entry=true, 
xid=xid@entry=63800060) at slru.c:610
#5  0x00533350 in SimpleLruReadPage_ReadOnly (ctl=ctl@entry=0xfbad00 
, pageno=pageno@entry=1947, xid=xid@entry=63800060) at slru.c:680
#6  0x005293fd in TransactionIdGetStatus (xid=xid@entry=63800060, 
lsn=lsn@entry=0x7ffd17fc5130) at clog.c:661
#7  0x0053574a in TransactionLogFetch (transactionId=63800060) at 
transam.c:79
#8  TransactionIdDidCommit (transactionId=transactionId@entry=63800060) at 
transam.c:129
#9 0x004f1295 in HeapTupleHeaderAdvanceLatestRemovedXid 
(tuple=0x2aab27e936e0, latestRemovedXid=latestRemovedXid@entry=0x7ffd17fc51b0) 
at heapam.c:7672
#10 0x005103e0 in btree_xlog_delete_get_latestRemovedXid 
(record=record@entry=0x4636c98) at nbtxlog.c:656
#11 0x00510a19 in btree_xlog_delete (record=0x4636c98) at nbtxlog.c:707
#12 btree_redo (record=0x4636c98) at nbtxlog.c:1048
#13 0x005544a1 in StartupXLOG () at xlog.c:7825
#14 0x008185be in StartupProcessMain () at startup.c:226
#15 0x0058de15 in AuxiliaryProcessMain (argc=argc@entry=2, 
argv=argv@entry=0x7ffd17fc9430) at bootstrap.c:448
#16 0x00813fe4 in StartChildProcess (type=StartupProcess) at 
postmaster.c:5804
#17 0x00817eb0 in PostmasterMain (argc=argc@entry=3, 
argv=argv@entry=0x45ed6e0) at postmaster.c:1461
#18 0x004991f4 in main (argc=3, argv=0x45ed6e0) at main.c:232
(gdb) p /x *record
$10 = {wal_segment_size = 0x4000, read_page = 0x54f920, system_identifier = 
0x5e6e6ea4af938064, private_data = 0x7ffd17fc5390, ReadRecPtr = 0xe41e8fb28, 
EndRecPtr = 0xe41e8fbb0, decoded_record = 0x4634390, main_data = 0x4634c88, 
main_data_len = 0x54,
  main_data_bufsz = 0x1000, record_origin = 0x0, blocks = {{in_use = 0x1, rnode 
= {spcNode = 0x67f, dbNode = 0x7517968, relNode = 0x7517e81}, forknum = 0x0, 
blkno = 0x55f, flags = 0x0, has_image = 0x0, apply_image = 0x0, bkp_image = 
0x4632bc1, hole_offset = 0x40,
  hole_length = 0x1fb0, bimg_len = 0x50, bimg_info = 0x5, has_data = 0x0, 
data = 0x4656938, data_len = 0x0, data_bufsz = 0x2000}, {in_use = 0x0, rnode = 
{spcNode = 0x67f, dbNode = 0x7517968, relNode = 0x7517e2b}, forknum = 0x0, 
blkno = 0x3b5f, flags = 0x80,
  has_image = 0x0, apply_image = 0x0, bkp_image = 0x0, hole_offset = 0x0, 
hole_length = 0x0, bimg_len = 0x0, bimg_info = 0x0, has_data = 0x0, data = 
0x46468e8, data_len = 0x0, data_bufsz = 0x2000}, {in_use = 0x0, rnode = 
{spcNode = 0x67f, dbNode = 0x7517968,
relNode = 0x7e5c370}, forknum = 0x0, blkno = 0x2c3, flags = 0x80, 
has_image = 0x0, apply_image = 0x0, bkp_image = 0x0, hole_offset = 0x0, 
hole_length = 0x0, bimg_len = 0x0, bimg_info = 0x0, has_data = 0x0, data = 0x0, 
data_len = 0x0, data_bufsz = 0x0}, {
  in_use = 0x0, rnode = {spcNode = 0x67f, dbNode = 0x7517968, relNode = 
0x7517e77}, forknum = 0x0, blkno = 0xa8a, flags = 0x80, has_image = 0x0, 
apply_image = 0x0, bkp_image = 0x0, hole_offset = 0x0, hole_length = 0x0, 
bimg_len = 0x0, bimg_info = 0x0,
  has_data = 0x0, data = 0x0, data_len = 0x0, data_bufsz = 0x0}, {in_use = 
0x0, rnode = {spcNode = 0x0, dbNode = 0x0, relNode = 0x0}, forknum = 0x0, blkno 
= 0x0, flags = 0x0, has_image = 0x0, apply_image = 0x0, bkp_image = 0x0, 
hole_offset = 0x0, hole_length = 0x0,
  bimg_len = 0x0, bimg_info = 0x0, has_data = 0x0, data = 0x0, data_len = 
0x0, data_bufsz = 0x0} }, max_block_id = 0x0, readBuf = 
0x4632868, readLen = 0x2000, readSegNo = 0x39, readOff = 0x1e8e000, readPageTLI 
= 0x1, latestPagePtr = 0xe41e8e000,
  latestPageTLI = 0x1, currRecPtr = 0xe41e8fb28, currTLI = 0x0, 
currTLIValidUntil = 0x0, nextTLI = 0x0, readRecordBuf = 0x463, 
readRecordBufSize = 0xa000, errormsg_buf = 0x4634878, noPayload = 0x0, 
polar_logindex_meta_size = 0x2e}
(gdb) p /x minRecoveryPoint
$11 = 0xe41bbbfd8
(gdb) p reachedConsistency
$12 = true
(gdb) p standbyState
$13 = STANDBY_SNAPSHOT_READY
(gdb) p ArchiveRecoveryRequested
$14 = true


After the crash the standby redo started f

Re: comments on elements of xlogctldata

[Fujii Masao]

On 2020/03/16 11:08, Fujii Masao wrote:



On 2020/03/16 10:57, Atsushi Torikoshi wrote:

Hi,

It seems the comments on SharedHotStandbyActive and SharedRecoveryInProgress 
are the same in XLogCtlData.

How about modifying the comment on SharedHotStandbyActive?

Attached a patch.


Thanks for the report and patch!
The patch looks good to me.
I will commit it.


Pushed! Thanks!

Regards,

--
Fujii Masao
NTT DATA CORPORATION
Advanced Platform Technology Group
Research and Development Headquarters

Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*)

[Justin Pryzby]

On Mon, Mar 16, 2020 at 07:17:36PM -0300, Alvaro Herrera wrote:
> I pushed 0001 and 0003 (as a single commit).  archive_statusdir didn't
> get here until 12, so your commit message was mistaken.  Also, pg10 is
> slightly different so it didn't apply there, so I left it alone.

Thanks, I appreciate it (and I'm sure Fabien will appreciate having two fewer
patches...).

@cfbot: rebased onto b4570d33aa045df330bb325ba8a2cbf02266a555

I realized that if I lstat() a file to make sure links to dirs show as
isdir=false, it's odd to then show size and timestamps of the dir.  So changed
to use lstat ... and squished.

-- 
Justin
>From d8294c4747c5ba1f3bec858c137cc2d31e5a0425 Mon Sep 17 00:00:00 2001
From: Justin Pryzby 
Date: Mon, 16 Mar 2020 14:12:55 -0500
Subject: [PATCH v13 1/8] Document historic behavior of links to directories..

Backpatch to 9.5: pg_stat_file
---
 doc/src/sgml/func.sgml | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index fc4d7f0f78..2c6142a0e0 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -21528,7 +21528,8 @@ SELECT convert_from(pg_read_binary_file('file_in_utf8.txt'), 'UTF8');
 size, last accessed time stamp, last modified time stamp,
 last file status change time stamp (Unix platforms only),
 file creation time stamp (Windows only), and a boolean
-indicating if it is a directory.  Typical usages include:
+indicating if it is a directory (or a symbolic link to a directory).
+Typical usages include:
 
 SELECT * FROM pg_stat_file('filename');
 SELECT (pg_stat_file('filename')).modification;
-- 
2.17.0

>From 27913ef889696ef2c42ed52bf7097e4a5aeaad9e Mon Sep 17 00:00:00 2001
From: Justin Pryzby 
Date: Mon, 9 Mar 2020 22:40:24 -0500
Subject: [PATCH v13 2/8] Add tests exercising pg_ls_tmpdir..

..and its backing function pg_ls_dir_files
---
 src/test/regress/expected/misc_functions.out | 7 +++
 src/test/regress/input/tablespace.source | 5 +
 src/test/regress/output/tablespace.source| 8 
 src/test/regress/sql/misc_functions.sql  | 4 
 4 files changed, 24 insertions(+)

diff --git a/src/test/regress/expected/misc_functions.out b/src/test/regress/expected/misc_functions.out
index d3acb98d04..903f1fe443 100644
--- a/src/test/regress/expected/misc_functions.out
+++ b/src/test/regress/expected/misc_functions.out
@@ -201,6 +201,13 @@ select count(*) > 0 from
  t
 (1 row)
 
+-- This tests the missing_ok parameter, which causes pg_ls_tmpdir to succeed even if the tmpdir doesn't exist yet
+-- The name='' condition is never true, so the function runs to completion but returns zero rows.
+select * from pg_ls_tmpdir() where name='Does not exist';
+ name | size | modification 
+--+--+--
+(0 rows)
+
 --
 -- Test adding a support function to a subject function
 --
diff --git a/src/test/regress/input/tablespace.source b/src/test/regress/input/tablespace.source
index a5f61a35dc..0b9cfe615e 100644
--- a/src/test/regress/input/tablespace.source
+++ b/src/test/regress/input/tablespace.source
@@ -11,6 +11,11 @@ DROP TABLESPACE regress_tblspacewith;
 -- create a tablespace we can use
 CREATE TABLESPACE regress_tblspace LOCATION '@testtablespace@';
 
+-- This tests the missing_ok parameter, which causes pg_ls_tmpdir to succeed even if the tmpdir doesn't exist yet
+-- The name='' condition is never true, so the function runs to completion but returns zero rows.
+-- The query is written to ERROR if the tablespace doesn't exist, rather than silently failing to call pg_ls_tmpdir()
+SELECT c.* FROM (SELECT oid FROM pg_tablespace b WHERE b.spcname='regress_tblspace' UNION SELECT 0 ORDER BY 1 DESC LIMIT 1) AS b , pg_ls_tmpdir(oid) AS c WHERE c.name='Does not exist';
+
 -- try setting and resetting some properties for the new tablespace
 ALTER TABLESPACE regress_tblspace SET (random_page_cost = 1.0, seq_page_cost = 1.1);
 ALTER TABLESPACE regress_tblspace SET (some_nonexistent_parameter = true);  -- fail
diff --git a/src/test/regress/output/tablespace.source b/src/test/regress/output/tablespace.source
index 162b591b31..a42714bf40 100644
--- a/src/test/regress/output/tablespace.source
+++ b/src/test/regress/output/tablespace.source
@@ -13,6 +13,14 @@ SELECT spcoptions FROM pg_tablespace WHERE spcname = 'regress_tblspacewith';
 DROP TABLESPACE regress_tblspacewith;
 -- create a tablespace we can use
 CREATE TABLESPACE regress_tblspace LOCATION '@testtablespace@';
+-- This tests the missing_ok parameter, which causes pg_ls_tmpdir to succeed even if the tmpdir doesn't exist yet
+-- The name='' condition is never true, so the function runs to completion but returns zero rows.
+-- The query is written to ERROR if the tablespace doesn't exist, rather than silently failing to call pg_ls_tmpdir()
+SELECT c.* FROM (SELECT oid FROM pg_tablespace b WHERE b.spcname='regress_tblspace' UNION SELECT 0 ORDER BY 1 DESC LIMIT 1) AS b , pg_ls_tmpdir(oid) AS c WHERE c.name='Does not e

Re: Autovacuum on partitioned table

[yuzuko]

Hello,

Thank you for reviewing.

> > > +  */
> > > + if (IsAutoVacuumWorkerProcess() &&
> > > + rel->rd_rel->relispartition &&
> > > + !(rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE))
> >
> > I'm not sure I understand why we do this only on autovac. Why not all
> > analyzes?
>
> +1.  If there is a reason, it should at least be documented in the
> comment above.
>
When we analyze partitioned table by ANALYZE command,
all inheritors including partitioned table are analyzed
at the same time.  In this case, if we call pgstat_report_partanalyze,
partitioned table's changes_since_analyze is updated
according to the number of analyzed tuples of partitions
as follows.  But I think it should be 0.

\d+ p
   Partitioned table "public.p"
 Column |  Type   | Collation | Nullable | Default | Storage | Stats
target | Description
+-+---+--+-+-+--+-
 i  | integer |   |  | | plain   |  |
Partition key: RANGE (i)
Partitions: p_1 FOR VALUES FROM (0) TO (100),
 p_2 FOR VALUES FROM (100) TO (200)

insert into p select * from generate_series(0,199);
INSERT 0 200

(before analyze)
-[ RECORD 1 ]---+--
relname | p
n_mod_since_analyze | 0
-[ RECORD 2 ]---+--
relname | p_1
n_mod_since_analyze | 100
-[ RECORD 3 ]---+--
relname | p_2
n_mod_since_analyze | 100

(after analyze)
-[ RECORD 1 ]---+--
relname | p
n_mod_since_analyze | 200
-[ RECORD 2 ]---+--
relname | p_1
n_mod_since_analyze | 0
-[ RECORD 3 ]---+--
relname | p_2
n_mod_since_analyze | 0


I think if we analyze partition tree in order from leaf partitions
to root table, this problem can be fixed.
What do you think about it?

-- 
Best regards,
Yuzuko Hosoya
NTT Open Source Software Center

Re: Re:Standby got fatal after the crash recovery

[Michael Paquier]

On Tue, Mar 17, 2020 at 11:02:24AM +0800, Thunder wrote:
> The slru error detail is the following.
> DETAIL:  Could not read from file "/data/pg_xact/003C" at offset 221184: 
> Success.
> 
> I think read /data/pg_xact/003C from offset 221184 and the return value was 0.

What is the version of PostgreSQL you are using here?  You have not
mentioned this information in any of the three emails you sent on this
thread.
--
Michael


signature.asc
Description: PGP signature

Re: [Proposal] Global temporary tables

[曾文旌(义从)]

> 2020年3月11日 下午3:52，Prabhat Sahu  写道：
> 
> On Mon, Mar 9, 2020 at 10:02 PM 曾文旌(义从)  > wrote:
> 
> 
> Fixed in global_temporary_table_v18-pg13.patch.
> Hi Wenjing,
> Thanks for the patch. I have verified the previous issues with 
> "gtt_v18_pg13.patch" and those are resolved.
> Please find below case:
> 
> postgres=# create sequence seq;
> CREATE SEQUENCE
> 
> postgres=# CREATE GLOBAL TEMPORARY TABLE gtt1(c1 int PRIMARY KEY) ON COMMIT 
> DELETE ROWS;
> CREATE TABLE
> 
> postgres=# CREATE GLOBAL TEMPORARY TABLE gtt2(c1 int PRIMARY KEY) ON COMMIT 
> PRESERVE ROWS;
> CREATE TABLE
> 
> postgres=# alter table gtt1 add c2 int default nextval('seq');
> ERROR:  cannot reindex global temporary tables
> 
> postgres=# alter table gtt2 add c2 int default nextval('seq');
> ERROR:  cannot reindex global temporary tables
reindex GTT is already supported

Please check global_temporary_table_v20-pg13.patch


Wenjing



> 
> Note: We are getting this error if we have a key column(PK/UNIQUE) in a GTT, 
> and trying to add a column with a default sequence into it.
> 
> -- 
> With Regards,
> Prabhat Kumar Sahu
> EnterpriseDB: http://www.enterprisedb.com 

Re: error context for vacuum to include block number

[Amit Kapila]

On Mon, Mar 16, 2020 at 7:47 PM Alvaro Herrera  wrote:
>
> On 2020-Mar-16, Amit Kapila wrote:
>
> > 2.
> > + /* Setup error traceback support for ereport() */
> > + update_vacuum_error_cbarg(vacrelstats, VACUUM_ERRCB_PHASE_SCAN_HEAP,
> > + InvalidBlockNumber, NULL);
> > + errcallback.callback = vacuum_error_callback;
> > + errcallback.arg = vacrelstats;
> > + errcallback.previous = error_context_stack;
> > + error_context_stack = &errcallback;
> > ..
> > ..
> > + /* Init vacrelstats for use as error callback by parallel worker: */
> > + vacrelstats.relnamespace = 
> > get_namespace_name(RelationGetNamespace(onerel));
> > + vacrelstats.relname = pstrdup(RelationGetRelationName(onerel));
> > + vacrelstats.indname = NULL;
> > + vacrelstats.phase = VACUUM_ERRCB_PHASE_UNKNOWN; /* Not yet processing */
> > +
> > + /* Setup error traceback support for ereport() */
> > + errcallback.callback = vacuum_error_callback;
> > + errcallback.arg = &vacrelstats;
> > + errcallback.previous = error_context_stack;
> > + error_context_stack = &errcallback;
> > +
> >
> > I think the code can be bit simplified if we have a function
> > setup_vacuum_error_ctx which takes necessary parameters and fill the
> > required vacrelstats params, setup errcallback.  Then we can use
> > update_vacuum_error_cbarg at required places.
>
> Heh, he had that and I took it away -- it looked unnatural.  I thought
> changing error_context_stack inside such a function, then resetting it
> back to "previous" outside the function, was too leaky an abstraction.
>

We could have something like setup_parser_errposition_callback and
cancel_parser_errposition_callback which might look a bit better.  I
thought to avoid having similar code at different places and it might
look a bit cleaner especially because we are adding code to an already
large function like lazy_scan_heap(), but if you don't like the idea,
then we can leave it as it is.

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

Re: error context for vacuum to include block number

[Justin Pryzby]

On Tue, Mar 03, 2020 at 10:05:42PM +0900, Masahiko Sawada wrote:
> I was concerned about fsm vacuum; vacuum error context might show heap
> scan while actually doing fsm vacuum. But perhaps we can update
> callback args for that. That would be helpful for user to distinguish
> that the problem seems to be either in heap vacuum or in fsm vacuum.

On Tue, Mar 03, 2020 at 04:49:00PM -0300, Alvaro Herrera wrote:
> Your use of the progress-report enum now has two warts -- the "-1"
> value, and this one,
> 
> > +#define PROGRESS_VACUUM_PHASE_VACUUM_FSM   7 /* For error 
> > reporting only */
> 
> I'd rather you define a new enum, in lazyvacuum.c.

On Mon, Mar 16, 2020 at 11:44:25AM +0530, Amit Kapila wrote:
> > On Wed, Mar 04, 2020 at 04:21:06PM +0900, Masahiko Sawada wrote:
> > > Thank you for updating the patch. But we have two more places where we
> > > do fsm vacuum.
> >
> > Oops, thanks.
...
> it is not clear whether it is a good idea to keep a phase like
> VACUUM_ERRCB_PHASE_VACUUM_FSM as it has added additional updates in
> multiple places in the code.

I think you're suggesting to rip out VACUUM_ERRCB_PHASE_VACUUM_FSM, and allow
reporting any errors there with an error context like "while scanning heap".

An alternative in the three places using VACUUM_ERRCB_PHASE_VACUUM_FSM is to
set:

|phase = VACUUM_ERRCB_PHASE_UNKNOWN;

to avoid reporting any error context until another phase is set.

-- 
Justin

Re:Re: Re:Standby got fatal after the crash recovery

[Thunder]

Sorry.
We are using pg11, and cloned from tag REL_11_BETA2.





















At 2020-03-17 11:43:51, "Michael Paquier"  wrote:
>On Tue, Mar 17, 2020 at 11:02:24AM +0800, Thunder wrote:
>> The slru error detail is the following.
>> DETAIL:  Could not read from file "/data/pg_xact/003C" at offset 221184: 
>> Success.
>> 
>> I think read /data/pg_xact/003C from offset 221184 and the return value was 
>> 0.
>
>What is the version of PostgreSQL you are using here?  You have not
>mentioned this information in any of the three emails you sent on this
>thread.
>--
>Michael

Re: Refactor compile-time assertion checks for C/C++

[Michael Paquier]

On Mon, Mar 16, 2020 at 10:35:05PM -0400, Tom Lane wrote:
> Michael Paquier  writes:
>> C++ does not allow defining a struct inside a sizeof() call, so in
>> this case StaticAssertExpr() does not work with the previous extension
>> in C++.  StaticAssertStmt() does the work though.
> 
> [ scratches head... ]  A do{} is okay in an expression in C++ ??

cpp-fallback-fix.patch in [1] was doing that.

The fun does not stop here.  gcc is fine when using that for C and
C++:
#define StaticAssertStmt(condition, errmessage) \
   do { struct static_assert_struct { int static_assert_failure : (condition) ? 
1 : -1; }; } while(0)
#define StaticAssertExpr(condition, errmessage) \
   ((void) ({ StaticAssertStmt(condition, errmessage); }))

But then problems come from MSVC which does not like the do{} part for
statements, and this works:
#define StaticAssertStmt(condition, errmessage) \
((void) sizeof(struct { int static_assert_failure : (condition) ? 1 : -1; 
}))
#define StaticAssertExpr(condition, errmessage) \
StaticAssertStmt(condition, errmessage)

[1]: https://postgr.es/m/20200313115033.ga183...@paquier.xyz
--
Michael


signature.asc
Description: PGP signature

Re: error context for vacuum to include block number

[Amit Kapila]

On Tue, Mar 17, 2020 at 9:21 AM Justin Pryzby  wrote:
>
> On Tue, Mar 03, 2020 at 10:05:42PM +0900, Masahiko Sawada wrote:
> > I was concerned about fsm vacuum; vacuum error context might show heap
> > scan while actually doing fsm vacuum. But perhaps we can update
> > callback args for that. That would be helpful for user to distinguish
> > that the problem seems to be either in heap vacuum or in fsm vacuum.
>
> On Tue, Mar 03, 2020 at 04:49:00PM -0300, Alvaro Herrera wrote:
> > Your use of the progress-report enum now has two warts -- the "-1"
> > value, and this one,
> >
> > > +#define PROGRESS_VACUUM_PHASE_VACUUM_FSM   7 /* For error 
> > > reporting only */
> >
> > I'd rather you define a new enum, in lazyvacuum.c.
>
> On Mon, Mar 16, 2020 at 11:44:25AM +0530, Amit Kapila wrote:
> > > On Wed, Mar 04, 2020 at 04:21:06PM +0900, Masahiko Sawada wrote:
> > > > Thank you for updating the patch. But we have two more places where we
> > > > do fsm vacuum.
> > >
> > > Oops, thanks.
> ...
> > it is not clear whether it is a good idea to keep a phase like
> > VACUUM_ERRCB_PHASE_VACUUM_FSM as it has added additional updates in
> > multiple places in the code.
>
> I think you're suggesting to rip out VACUUM_ERRCB_PHASE_VACUUM_FSM, and allow
> reporting any errors there with an error context like "while scanning heap".
>

Right, because that is what we do for progress updates.

> An alternative in the three places using VACUUM_ERRCB_PHASE_VACUUM_FSM is to
> set:
>
> |phase = VACUUM_ERRCB_PHASE_UNKNOWN;
>
> to avoid reporting any error context until another phase is set.
>

Right, that is an alternative, but not sure if it is worth adding
additional code.  I am trying to see if we can get this functionality
without adding code at too many places primarily because the code in
this area is already complex, so adding more things can make it
difficult to understand.

Another minor point, don't we need to remove the error stack by doing
"error_context_stack = errcallback.previous;" in parallel_vacuum_main?

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