Re: [PATCH] Do not use StdRdOptions in Access Methods

[Michael Paquier]

On Wed, Nov 13, 2019 at 04:26:53PM +0300, Nikolay Shaplov wrote:
> I've changed the patch to use build_reloptions function and again propose it 
> to the commitfest.

Thanks for the new patch.  I have not reviewed the patch in details,
but I have a small comment.

> +#define SpGistGetFillFactor(relation) \
> + ((relation)->rd_options ? \
> + ((SpGistOptions *) (relation)->rd_options)->fillfactor : \
> + SPGIST_DEFAULT_FILLFACTOR)
> +
Wouldn't it make sense to add assertions here to make sure that the
relkind is an index?  You basically did that in commit 3967737.
--
Michael


signature.asc
Description: PGP signature

Re: Built-in connection pooler

[Konstantin Knizhnik]

For now I replay for the above. Oh sorry, I was wrong about the condition.
The error occurred under following condition.
- port = 32768
- proxy_port = 6543
- $ psql postgres -p 6543

$ bin/pg_ctl start -D data
waiting for server to start
  LOG:  starting PostgreSQL 13devel on x86_64-pc-linux-gnu, compiled by gcc 
(GCC) 4.8.5 20150623 (Red Hat 4.8.5-28), 64-bit
  LOG:  listening on IPv6 address "::1", port 6543
  LOG:  listening on IPv4 address "127.0.0.1", port 6543
  LOG:  listening on IPv6 address "::1", port 32768
  LOG:  listening on IPv4 address "127.0.0.1", port 32768
  LOG:  listening on Unix socket "/tmp/.s.PGSQL.6543"
  LOG:  listening on Unix socket "/tmp/.s.PGSQL.32768"
  LOG:  Start proxy process 25374
  LOG:  Start proxy process 25375
  LOG:  database system was shut down at 2019-11-12 16:49:20 JST
  LOG:  database system is ready to accept connections

server started
[postgres@vm-7kfq-coreban connection-pooling]$ psql -p 6543
LOG:  Message size 84
WARNING:  could not setup local connect to server
DETAIL:  invalid port number: "-32768"
LOG:  Handshake response will be sent to the client later when backed is 
assigned
psql: error: could not connect to server: invalid port number: "-32768"

By the way, the patch has some small conflicts against master.


Thank you very much for reporting the problem.
It was caused by using pg_itoa for string representation of port (I 
could not imagine that unlike standard itoa it accepts int16 parameter 
instead of int).

Attached please find rebased patch with this bug fixed.

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

diff --git a/contrib/spi/refint.c b/contrib/spi/refint.c
index adf0490..5c2095f 100644
--- a/contrib/spi/refint.c
+++ b/contrib/spi/refint.c
@@ -11,6 +11,7 @@
 
 #include "commands/trigger.h"
 #include "executor/spi.h"
+#include "storage/proc.h"
 #include "utils/builtins.h"
 #include "utils/rel.h"
 
@@ -93,6 +94,8 @@ check_primary_key(PG_FUNCTION_ARGS)
 	else
 		tuple = trigdata->tg_newtuple;
 
+	MyProc->is_tainted = true;
+
 	trigger = trigdata->tg_trigger;
 	nargs = trigger->tgnargs;
 	args = trigger->tgargs;
@@ -284,6 +287,8 @@ check_foreign_key(PG_FUNCTION_ARGS)
 		/* internal error */
 		elog(ERROR, "check_foreign_key: cannot process INSERT events");
 
+	MyProc->is_tainted = true;
+
 	/* Have to check tg_trigtuple - tuple being deleted */
 	trigtuple = trigdata->tg_trigtuple;
 
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index f837703..7433e6f 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -732,6 +732,169 @@ include_dir 'conf.d'
   
  
 
+ 
+  max_sessions (integer)
+  
+   max_sessions configuration parameter
+  
+  
+  
+
+  The maximum number of client sessions that can be handled by
+  one connection proxy when session pooling is enabled.
+  This parameter does not add any memory or CPU overhead, so
+  specifying a large max_sessions value
+  does not affect performance.
+  If the max_sessions limit is reached new connections are not accepted.
+
+
+  The default value is 1000. This parameter can only be set at server start.
+   
+  
+ 
+
+ 
+  session_pool_size (integer)
+  
+   session_pool_size configuration parameter
+  
+  
+  
+
+  Enables session pooling and defines the maximum number of
+  backends that can be used by client sessions for each database/user combination.
+  Launched non-tainted backends are never terminated even if there are no active sessions.
+  Backend is considered as tainted if client updates GUCs, creates temporary table or prepared statements.
+  Tainted backend can server only one client.
+
+
+  The default value is 10, so up to 10 backends will serve each database,
+
+  
+ 
+
+ 
+  proxy_port (integer)
+  
+   proxy_port configuration parameter
+  
+  
+  
+
+  Sets the TCP port for the connection pooler.
+  Clients connected to main "port" will be assigned dedicated backends,
+  while client connected to proxy port will be connected to backends through proxy which
+  performs transaction level scheduling. 
+   
+
+  The default value is 6543.
+
+  
+ 
+
+ 
+  connection_proxies (integer)
+  
+   connection_proxies configuration parameter
+  
+  
+  
+
+  Sets number of connection proxies.
+  Postmaster spawns separate worker process for each proxy. Postmaster scatters connections between proxies using one of scheduling policies (round-robin, random, load-balancing).
+  Each proxy launches its own subset of backends.
+  So maximal number of non-tainted backends is  

Re: dropdb --force

[Pavel Stehule]

čt 14. 11. 2019 v 7:44 odesílatel Amit Kapila 
napsal:

> On Thu, Nov 14, 2019 at 11:43 AM Pavel Stehule 
> wrote:
> >
> > st 13. 11. 2019 v 15:34 odesílatel Pavel Stehule <
> pavel.steh...@gmail.com> napsal:
> >>
> >>
> >> here it is. It's based on Filip Rembialkowski's patch if I remember
> correctly
> >
> >
> > have I this patch assign to next commitfest or you process it in this
> commitfest?
> >
>
> I will try to review in this CF only, but if I fail to do so, any way
> you can register it in new CF after this CF.
>

ok.

there is enough long time to do.

Regards

Pavel

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

Re: dropdb --force

[Amit Kapila]

On Thu, Nov 14, 2019 at 12:14 PM Amit Kapila  wrote:
>
> On Thu, Nov 14, 2019 at 11:43 AM Pavel Stehule  
> wrote:
> >
> > st 13. 11. 2019 v 15:34 odesílatel Pavel Stehule  
> > napsal:
> >>
> >>
> >> here it is. It's based on Filip Rembialkowski's patch if I remember 
> >> correctly
> >
> >
> > have I this patch assign to next commitfest or you process it in this 
> > commitfest?
> >
>
> I will try to review in this CF only,
>

This is the reason I haven't yet marked the CF entry for this patch as
committed.

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

Re: dropdb --force

[Amit Kapila]

On Thu, Nov 14, 2019 at 11:43 AM Pavel Stehule  wrote:
>
> st 13. 11. 2019 v 15:34 odesílatel Pavel Stehule  
> napsal:
>>
>>
>> here it is. It's based on Filip Rembialkowski's patch if I remember correctly
>
>
> have I this patch assign to next commitfest or you process it in this 
> commitfest?
>

I will try to review in this CF only, but if I fail to do so, any way
you can register it in new CF after this CF.

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

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

[Amit Kapila]

On Thu, Nov 14, 2019 at 9:37 AM Dilip Kumar  wrote:
>
> On Wed, Nov 13, 2019 at 5:55 PM Amit Kapila  wrote:
> >
> > On Thu, Oct 3, 2019 at 1:18 PM Dilip Kumar  wrote:
> > >
> >
> > As mentioned by me a few days back that the first patch in this series
> > is ready to go [1] (I am hoping Tomas will pick it up), so I have
> > started the review of other patches
> >
> > Review/Questions on 0002-Immediately-WAL-log-assignments.patch
> > -
> > 1. This patch adds the top_xid in WAL whenever the first time WAL for
> > a subtransaction XID is written to correctly decode the changes of
> > in-progress transaction.  This patch also removes logging and applying
> > WAL for XLOG_XACT_ASSIGNMENT which might have some effect.  As replay
> > of that, it prunes KnownAssignedXids to prevent overflow of that
> > array.  See comments in procarray.c (KnownAssignedTransactionIds
> > sub-module).  Can you please explain how after removing the WAL for
> > XLOG_XACT_ASSIGNMENT, we will handle that or I am missing something
> > and there is no impact of same?
>
> It seems like a problem to me as well.   One option could be that
> since now we are adding the top transaction id in the first WAL of the
> subtransaction we can directly update the pg_subtrans and avoid adding
> sub transaction id in the KnownAssignedXids and mark it as
> lastOverflowedXid.
>

Hmm, I am not sure if we can do that easily because I think in
RecordKnownAssignedTransactionIds, we add those based on the gap via
KnownAssignedXidsAdd and only remove them later while applying WAL for
XLOG_XACT_ASSIGNMENT.  I think if we really want to go in this
direction then for each WAL record we need to check if it has
XLR_BLOCK_ID_TOPLEVEL_XID set and then call function
ProcArrayApplyXidAssignment() with the required information.  I think
this line of attack has WAL overhead both on master whenever
subtransactions are involved and also on hot-standby for doing the
work for each subtransaction separately.  The WAL apply needs to
acquire and release PROCArrayLock in exclusive mode for each
subtransaction whereas now it does it once for
PGPROC_MAX_CACHED_SUBXIDS number of subtransactions which can conflict
with queries running on standby.

The other idea could be that we keep the current XLOG_XACT_ASSIGNMENT
mechanism (WAL logging and apply of same on hot-standby) as it is and
additionally log top_xid the first time when WAL is written for a
subtransaction only when wal_level >= WAL_LEVEL_LOGICAL.  Then use the
same for logical decoding.  The advantage of this approach is that we
will incur the overhead of additional transactionid only when required
especially not with default server configuration.

Thoughts?

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

Re: Invisible PROMPT2

[Kyotaro Horiguchi]

At Wed, 13 Nov 2019 20:57:04 +0100, David Fetter  wrote in 
> On Wed, Nov 13, 2019 at 03:58:38PM -0300, Alvaro Herrera wrote:
> > On 2019-Nov-13, David Fetter wrote:
> > 
> > > On Wed, Nov 13, 2019 at 03:06:08PM -0300, Alvaro Herrera wrote:
> > > > On 2019-Nov-13, David Fetter wrote:
> > > > 
> > > > > On Wed, Nov 13, 2019 at 09:47:01AM -0500, Tom Lane wrote:
> > > > 
> > > > > > > How about a circumfix directive (like the existing %[ ... %])
> > > > > > > that replaces everything inside with whitespace, but keeps the 
> > > > > > > width?
> > 
> > > > This seems way too specific to me.  I like the "circumfix" directive
> > > > better, because it allows one to do more things.  I don't have any
> > > > immediate use for it, but it doesn't seem completely far-fetched that
> > > > there are some.
> > 
> > > So something like %w[...%w] where people could put things like PROMPT1
> > > inside?
> > 
> > Hmm, (I'm not sure your proposed syntax works, but let's assume that
> > it does.)  I'm saying you'd define
> > \set PROMPT1 '%a%b%c '
> > \set PROMPT2 '%w[%a%b%c %w]'
> > 
> > and you'd end up with matching indentation on multiline queries.

This seems assuming %x are a kind of stable (until semicolon)
function. But at least %`..` can be volatile.  So, I think the %w
thing in PROMPT2 should be able to refer the actual prompt string
resulted from PROMPT1.

> > I'm not sure that we'd need to make something like this work:
> >   PROMPT1="%w[$PROMPT1%w]"
> > which I think is what you're saying.
> 
> PROMPT2="%w[$PROMPT1%w]", and basically yes.

Like this. Or may be a bit too-much and I don't came up with a
lialistic use-case, but I think of the following syntax.

\set PROMPT1 '%w[%a%b%c%w] '
\set PROMPT2 '%w '

where %w in PROMPT2 is replaced by a whitespace with the same length
to the output of %w[..%w] part in PROMPT1.

> > We already have "%:PROMPT1:" but that expands to the literal value of
> > prompt1, not to the value that prompt1 would expand to:
> 
> Yeah, that's not so great for this usage.  I guess "expand variables"
> could be a separate useful feature (and patch) all by itself...

+1.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: dropdb --force

[Pavel Stehule]

st 13. 11. 2019 v 15:34 odesílatel Pavel Stehule 
napsal:

>
>
> st 13. 11. 2019 v 7:13 odesílatel Pavel Stehule 
> napsal:
>
>>
>>
>> st 13. 11. 2019 v 7:12 odesílatel Amit Kapila 
>> napsal:
>>
>>> On Tue, Nov 12, 2019 at 11:17 AM Amit Kapila 
>>> wrote:
>>> >
>>> > I am planning to commit this patch tomorrow unless I see more comments
>>> > or interest from someone else to review this.
>>> >
>>>
>>> Pushed.  Pavel, feel free to submit dropdb utility-related patch if you
>>> want.
>>>
>>
>> I hope I send this patch today. It's simply job.
>>
>
> here it is. It's based on Filip Rembialkowski's patch if I remember
> correctly
>

have I this patch assign to next commitfest or you process it in this
commitfest?

This part is trivial

Pavel


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

Re: [PATCH] Do not use StdRdOptions in Access Methods

[Michael Paquier]

On Wed, Nov 13, 2019 at 04:05:20PM +0900, Michael Paquier wrote:
> On Wed, Nov 13, 2019 at 02:29:49PM +0900, Amit Langote wrote:
> > On Wed, Nov 13, 2019 at 2:18 PM Michael Paquier  wrote:
> >> There could be an argument for keeping
> >> GET_STRING_RELOPTION actually which is still useful to get a string
> >> value in an option set using the stored offset, and we have
> >> the recently-added dummy_index_am in this category.  Any thoughts?
> > 
> > Not sure, maybe +0.5 on keeping GET_STRING_RELOPTION.
> 
> Thinking more about it, I would tend to keep this one around.  I'll
> wait a couple of days before coming back to it.

Committed this one and kept GET_STRING_RELOPTION().  With the removal
of those macros, it is possible to actually move a portion of the
parsing definitions to reloptions.c for each type, but as we expose
the validation function string and the enum element definition that
would be more confusing IMO, so I have left that out.
--
Michael


signature.asc
Description: PGP signature

Re: [HACKERS] Block level parallel vacuum

[Amit Kapila]

On Wed, Nov 13, 2019 at 9:51 AM Dilip Kumar  wrote:
>
> + /*
> + * Since parallel workers cannot access data in temporary tables, parallel
> + * vacuum is not allowed for temporary relation.
> + */
> + if (RelationUsesLocalBuffers(onerel) && params->nworkers >= 0)
> + {
> + ereport(WARNING,
> + (errmsg("skipping vacuum on \"%s\" --- cannot vacuum temporary
> tables in parallel",
> + RelationGetRelationName(onerel;
> + relation_close(onerel, lmode);
> + PopActiveSnapshot();
> + CommitTransactionCommand();
> + /* It's OK to proceed with ANALYZE on this table */
> + return true;
> + }
> +
>
> If we can not support the parallel vacuum for the temporary table then
> shouldn't we fall back to the normal vacuum instead of skipping the
> table.  I think it's not fair that if the user has given system-wide
> parallel vacuum then all the temp table will be skipped and not at all
> vacuumed then user need to again perform normal vacuum on those
> tables.
>

Good point.  However, I think the current coding also makes sense for
cases like "Vacuum (analyze, parallel 2) tmp_tab;".  In such a case,
it will skip the vacuum part of it but will perform analyze.  Having
said that, I can see the merit of your point and I also vote to follow
your suggestion and add a note to the document unless it makes code
look ugly.

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

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

[Dilip Kumar]

On Wed, Nov 13, 2019 at 5:55 PM Amit Kapila  wrote:
>
> On Thu, Oct 3, 2019 at 1:18 PM Dilip Kumar  wrote:
> >
>
> As mentioned by me a few days back that the first patch in this series
> is ready to go [1] (I am hoping Tomas will pick it up), so I have
> started the review of other patches
>
> Review/Questions on 0002-Immediately-WAL-log-assignments.patch
> -
> 1. This patch adds the top_xid in WAL whenever the first time WAL for
> a subtransaction XID is written to correctly decode the changes of
> in-progress transaction.  This patch also removes logging and applying
> WAL for XLOG_XACT_ASSIGNMENT which might have some effect.  As replay
> of that, it prunes KnownAssignedXids to prevent overflow of that
> array.  See comments in procarray.c (KnownAssignedTransactionIds
> sub-module).  Can you please explain how after removing the WAL for
> XLOG_XACT_ASSIGNMENT, we will handle that or I am missing something
> and there is no impact of same?

It seems like a problem to me as well.   One option could be that
since now we are adding the top transaction id in the first WAL of the
subtransaction we can directly update the pg_subtrans and avoid adding
sub transaction id in the KnownAssignedXids and mark it as
lastOverflowedXid.  But, I don't think we should go in that direction
otherwise it will impact the performance of visibility check on the
hot-standby.  Let's see what Tomas has in mind.

>
> 2.
> +#define XLOG_INCLUDE_INVALS 0x08 /* include invalidations */
>
> This doesn't seem to be used in this patch.

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

Re: [PATCH] use separate PartitionedRelOptions structure to store partitioned table options

[Michael Paquier]

On Wed, Nov 13, 2019 at 05:02:24PM +0300, Nikolay Shaplov wrote:
> I did not read that thread yet, when I sent v3 patch.
> build_reloptions is a good stuff and we should use it for sure.
> 
> I've looked at yours v4 version of the patch, it is exactly what we need 
> here. 
> Can we commit it as it is?

I have done an extra lookup, removing PartitionedRelOptions because we
have no need for it yet, and committed the split. 
--
Michael


signature.asc
Description: PGP signature

Re: MarkBufferDirtyHint() and LSN update

[Michael Paquier]

On Wed, Nov 13, 2019 at 09:17:03PM +0900, Michael Paquier wrote:
> Actually, no, this is not good.  I have been studying more the patch,
> and after stressing more this code path with a cluster having
> checksums enabled and shared_buffers at 1MB, I have been able to make
> a couple of page's LSNs go backwards with pgbench -s 100.  The cause
> was simply that the page got flushed with a newer LSN than what was
> returned by XLogSaveBufferForHint() before taking the buffer header
> lock, so updating only the LSN for a non-dirty page was simply
> guarding against that.

for the reference attached is the trick I have used, adding an extra
assertion check in PageSetLSN(). 
--
Michael
diff --git a/src/include/storage/bufpage.h b/src/include/storage/bufpage.h
index 4ef6d8ddd4..3a7c4051c0 100644
--- a/src/include/storage/bufpage.h
+++ b/src/include/storage/bufpage.h
@@ -366,7 +366,10 @@ PageValidateSpecialPointer(Page page)
 #define PageGetLSN(page) \
 	PageXLogRecPtrGet(((PageHeader) (page))->pd_lsn)
 #define PageSetLSN(page, lsn) \
-	PageXLogRecPtrSet(((PageHeader) (page))->pd_lsn, lsn)
+	do { \
+		Assert(PageGetLSN(page) <= lsn); \
+		PageXLogRecPtrSet(((PageHeader) (page))->pd_lsn, lsn); \
+	} while(0)
 
 #define PageHasFreeLinePointers(page) \
 	(((PageHeader) (page))->pd_flags & PD_HAS_FREE_LINES)


signature.asc
Description: PGP signature

Re: Ought to use heap_multi_insert() for pg_attribute/depend insertions?

[Andres Freund]

Hi,

On 2019-11-13 15:58:28 +0900, Michael Paquier wrote:
> On Tue, Nov 12, 2019 at 10:33:16AM -0800, Andres Freund wrote:
> > On 2019-11-12 16:25:06 +0100, Daniel Gustafsson wrote:
> >> On 11 Nov 2019, at 09:32, Michael Paquier  wrote:
> >> 
> >>> This part has resulted in 75c1921, and we could just change
> >>> DecodeMultiInsert() so as if there is no tuple data then we'd just
> >>> leave.  However, I don't feel completely comfortable with that either
> >>> as it would be nice to still check that for normal relations we
> >>> *always* have a FPW available.
> >> 
> >> XLH_INSERT_CONTAINS_NEW_TUPLE will only be set in case of catalog relations
> >> IIUC (that is, non logically decoded relations), so it seems to me that we 
> >> can
> >> have a fastpath out of DecodeMultiInsert() which inspects that flag without
> >> problems. Is this proposal along the lines of what you were thinking?
> > 
> > Maybe I'm missing something, but it's the opposite, no?
> 
> > boolneed_tuple_data = RelationIsLogicallyLogged(relation);
> 
> Yep.  This checks after IsCatalogRelation().
> 
> > ...
> > if (need_tuple_data)
> > xlrec->flags |= XLH_INSERT_CONTAINS_NEW_TUPLE;
> > 
> > or am I misunderstanding what you mean?
> 
> Not sure that I can think about a good way to properly track if the
> new tuple data is associated to a catalog or not, aka if the data is
> expected all the time or not to get a good sanity check when doing the
> multi-insert decoding.  We could extend xl_multi_insert_tuple with a
> flag to track that, but that seems like an overkill for a simple
> sanity check..

My point was that I think there must be negation missing in Daniel's
statement - XLH_INSERT_CONTAINS_NEW_TUPLE will only be set if *not* a
catalog relation. But Daniel's statement says exactly the opposite, at
least by my read.

I can't follow what you're trying to get at in this sub discussion - why
would we want to sanity check something about catalog tables here? Given
that XLH_INSERT_CONTAINS_NEW_TUPLE is set exactly when the table is not
a catalog table, that seems entirely superfluous?

Greetings,

Andres Freund

Re: Missing dependency tracking for TableFunc nodes

[Mark Dilger]

On 11/13/19 4:46 PM, Tomas Vondra wrote:

On Wed, Nov 13, 2019 at 03:00:03PM -0800, Mark Dilger wrote:



On 11/11/19 1:41 PM, Tom Lane wrote:

I happened to notice that find_expr_references_walker has not
been taught anything about TableFunc nodes, which means it will
miss the type and collation OIDs embedded in such a node.

This can be demonstrated to be a problem by the attached script,
which will end up with a "cache lookup failed for type N"
error because we allow dropping a type the XMLTABLE construct
references.

This isn't hard to fix, as per the attached patch, but it makes
me nervous.  I wonder what other dependencies we might be missing.


I can consistently generate errors like the following in master:

 ERROR:  cache lookup failed for statistics object 31041

This happens in a stress test in which multiple processes are making 
changes to the schema.  So far, all the sessions that report this 
cache lookup error do so when performing one of ANALYZE, VACUUM 
ANALYZE, UPDATE, DELETE or EXPLAIN ANALYZE on a table that has MCV 
statistics. All processes running simultaneously are running the same 
set of functions, which create and delete tables, indexes, and 
statistics objects, insert, update, and delete rows in those tables, etc.


The fact that the statistics are of the MCV type might not be 
relevant; I'm creating those on tables as part of testing Tomas 
Vondra's MCV statistics patch, so all the tables have statistics of 
that kind on them.




Hmmm, I don't know the details of the test, but this seems a bit like
we're trying to use the stats during estimation but it got dropped
meanwhile. If that's the case, it probably affects all stats types, not
just MCV lists - there should no significant difference between
different statistics types, I think.

I've managed to reproduce this with a stress-test, and I do get these
failures with both dependencies and mcv stats, although in slightly
different places.

And I think I see the issue - when dropping the statistics, we do
RemoveObjects which however does not acquire any lock on the table. So
we get the list of stats (without the serialized data), but before we
get to load the contents, someone drops it. If that's the root cause,
it's there since pg 10.

I'm not sure what's the right solution. An straightforward option would
be to lock the relation, but will that work after adding support for
stats on joins? An alternative would be to just ignore those failures,
but that kinda breaks the estimation (we should have picked a different
stats in this case).

I can try to distill my test case a bit, but first I'd like to know if 
you are interested.  Currently, the patch is over 2.2MB, gzip'd.  I'll 
only bother distilling it if you don't already know about these cache 
lookup failures.




Not sure. But I do wonder if we see the same issue.


I don't know.  If you want to reproduce what I'm seeing

I added a parallel_schedule target:

diff --git a/src/test/regress/parallel_schedule 
b/src/test/regress/parallel_schedule

index fc0f14122b..5ace7c7a8a 100644
--- a/src/test/regress/parallel_schedule
+++ b/src/test/regress/parallel_schedule
@@ -85,6 +85,8 @@ test: create_table_like alter_generic alter_operator 
misc async dbsize misc_func

 # collate.*.utf8 tests cannot be run in parallel with each other
 test: rules psql psql_crosstab amutils stats_ext collate.linux.utf8

+test: mcv_huge_stress_a mcv_huge_stress_b mcv_huge_stress_c 
mcv_huge_stress_d mcv_huge_stress_e mcv_huge_stress_f mcv_huge_stress_g

+
 # run by itself so it can run parallel workers
 test: select_parallel
 test: write_parallel


And used the attached script to generate the contents of the seven 
parallel tests.  If you want to duplicate this, you'll have to manually 
run gen.pl and direct its output to those src/test/regress/sql/ files. 
The src/test/regress/expected/ files are just empty, as I don't care 
about whether the test results match.  I'm just checking what kinds of 
errors I get and whether any of them are concerning.


After my most recent run of the stress tests, I grep'd for cache 
failures and got 23 of them, all coming from get_relation_statistics(), 
statext_store() and statext_mcv_load().  Two different adjacent spots in 
get_relation_statistics() were involved:


htup = SearchSysCache1(STATEXTOID, ObjectIdGetDatum(statOid));
if (!HeapTupleIsValid(htup))
elog(ERROR, "cache lookup failed for statistics object %u", 
statOid);

staForm = (Form_pg_statistic_ext) GETSTRUCT(htup);

dtup = SearchSysCache1(STATEXTDATASTXOID, 
ObjectIdGetDatum(statOid));

if (!HeapTupleIsValid(dtup))
elog(ERROR, "cache lookup failed for statistics object %u", 
statOid);


Most were from the first SearchSysCache1 call, but one of them was from 
the second.


--
Mark Dilger


gen.pl
Description: Perl program

Re: Missing dependency tracking for TableFunc nodes

[Tom Lane]

Mark Dilger  writes:
> On 11/11/19 1:41 PM, Tom Lane wrote:
>> I happened to notice that find_expr_references_walker has not
>> been taught anything about TableFunc nodes, which means it will
>> miss the type and collation OIDs embedded in such a node.

> I can consistently generate errors like the following in master:
>ERROR:  cache lookup failed for statistics object 31041

This is surely a completely different issue --- there are not,
one hopes, any extended-stats OIDs embedded in views or other
query trees.

I concur with Tomas' suspicion that this must be a race condition
during DROP STATISTICS.  If we're going to allow people to do that
separately from dropping the table(s), there has to be some kind of
locking around it, and it sounds like there's not :-(

regards, tom lane

RE: Built-in connection pooler

[ideriha.take...@fujitsu.com]

Hi

>From: Konstantin Knizhnik [mailto:k.knizh...@postgrespro.ru]
>>> From: Konstantin Knizhnik [mailto:k.knizh...@postgrespro.ru]
>>>
>>> New version of builtin connection pooler fixing handling messages of
>>> extended protocol.
>>>
>> 2. When proxy_port is a bit large (perhaps more than 2^15), connection
>> failed though regular "port" is fine with number more than 2^15.
>>
>> $ bin/psql -p  32768
>> 2019-11-12 16:11:25.460 JST [5617] LOG:  Message size 84
>> 2019-11-12 16:11:25.461 JST [5617] WARNING:  could not setup local
>> connect to server
>> 2019-11-12 16:11:25.461 JST [5617] DETAIL:  invalid port number: "-22768"
>> 2019-11-12 16:11:25.461 JST [5617] LOG:  Handshake response will be
>> sent to the client later when backed is assigned
>> psql: error: could not connect to server: invalid port number: "-22768"
>Hmmm, ProxyPortNumber is used exactly in the same way as PostPortNumber.
>I was able to connect to the specified port:
>
>
>knizhnik@knizhnik:~/dtm-data$ psql postgres -p 42768 psql (13devel) Type 
>"help" for
>help.
>
>postgres=# \q
>knizhnik@knizhnik:~/dtm-data$ psql postgres -h 127.0.0.1 -p 42768 psql 
>(13devel)
>Type "help" for help.
>
>postgres=# \q

For now I replay for the above. Oh sorry, I was wrong about the condition.
The error occurred under following condition.
- port = 32768
- proxy_port = 6543
- $ psql postgres -p 6543

$ bin/pg_ctl start -D data
waiting for server to start
 LOG:  starting PostgreSQL 13devel on x86_64-pc-linux-gnu, compiled by gcc 
(GCC) 4.8.5 20150623 (Red Hat 4.8.5-28), 64-bit
 LOG:  listening on IPv6 address "::1", port 6543
 LOG:  listening on IPv4 address "127.0.0.1", port 6543
 LOG:  listening on IPv6 address "::1", port 32768
 LOG:  listening on IPv4 address "127.0.0.1", port 32768
 LOG:  listening on Unix socket "/tmp/.s.PGSQL.6543"
 LOG:  listening on Unix socket "/tmp/.s.PGSQL.32768"
 LOG:  Start proxy process 25374
 LOG:  Start proxy process 25375
 LOG:  database system was shut down at 2019-11-12 16:49:20 JST
 LOG:  database system is ready to accept connections

server started
[postgres@vm-7kfq-coreban connection-pooling]$ psql -p 6543
LOG:  Message size 84
WARNING:  could not setup local connect to server
DETAIL:  invalid port number: "-32768"
LOG:  Handshake response will be sent to the client later when backed is 
assigned
psql: error: could not connect to server: invalid port number: "-32768"

By the way, the patch has some small conflicts against master.

Regards,
Takeshi Ideriha

Re: Missing dependency tracking for TableFunc nodes

[Tomas Vondra]

On Wed, Nov 13, 2019 at 03:00:03PM -0800, Mark Dilger wrote:



On 11/11/19 1:41 PM, Tom Lane wrote:

I happened to notice that find_expr_references_walker has not
been taught anything about TableFunc nodes, which means it will
miss the type and collation OIDs embedded in such a node.

This can be demonstrated to be a problem by the attached script,
which will end up with a "cache lookup failed for type N"
error because we allow dropping a type the XMLTABLE construct
references.

This isn't hard to fix, as per the attached patch, but it makes
me nervous.  I wonder what other dependencies we might be missing.


I can consistently generate errors like the following in master:

 ERROR:  cache lookup failed for statistics object 31041

This happens in a stress test in which multiple processes are making 
changes to the schema.  So far, all the sessions that report this 
cache lookup error do so when performing one of ANALYZE, VACUUM 
ANALYZE, UPDATE, DELETE or EXPLAIN ANALYZE on a table that has MCV 
statistics. All processes running simultaneously are running the same 
set of functions, which create and delete tables, indexes, and 
statistics objects, insert, update, and delete rows in those tables, 
etc.


The fact that the statistics are of the MCV type might not be 
relevant; I'm creating those on tables as part of testing Tomas 
Vondra's MCV statistics patch, so all the tables have statistics of 
that kind on them.




Hmmm, I don't know the details of the test, but this seems a bit like
we're trying to use the stats during estimation but it got dropped
meanwhile. If that's the case, it probably affects all stats types, not
just MCV lists - there should no significant difference between
different statistics types, I think.

I've managed to reproduce this with a stress-test, and I do get these
failures with both dependencies and mcv stats, although in slightly
different places.

And I think I see the issue - when dropping the statistics, we do
RemoveObjects which however does not acquire any lock on the table. So
we get the list of stats (without the serialized data), but before we
get to load the contents, someone drops it. If that's the root cause,
it's there since pg 10.

I'm not sure what's the right solution. An straightforward option would
be to lock the relation, but will that work after adding support for
stats on joins? An alternative would be to just ignore those failures,
but that kinda breaks the estimation (we should have picked a different
stats in this case).

I can try to distill my test case a bit, but first I'd like to know if 
you are interested.  Currently, the patch is over 2.2MB, gzip'd.  I'll 
only bother distilling it if you don't already know about these cache 
lookup failures.




Not sure. But I do wonder if we see the same issue.

regards

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

Re: tuplesort test coverage

[Andres Freund]

Hi,

On 2019-10-25 12:37:38 +0100, Peter Geoghegan wrote:
> On Thu, Oct 24, 2019 at 10:10 PM Andres Freund  wrote:
> > Here's a first stab at getting the coverage of tuplesort.c to a
> > satisfying level.  There's still bits uncovered, but that's largely
> > either a) trace_sort related b) hopefully unreachable stuff c) explain
> > related. The largest actually missing thing is a disk-based
> > mark/restore, which probably ought be covered.
> 
> Yeah. It looks like function coverage of logtape.c will be 100% once
> you have coverage of mark and restore.

Yea, it's definitely better after.


> > I think the the test time of this would still be OK, but if not we could
> > also work a bit more on that angle.
> 
> That's hard for me to test right now, but offhand this general
> approach looks good to me. I am pretty sure it's portable.

I pushed this now. We'll see what the slower buildfarm animals say. I'll
try to see how long they took in a few days.

Greetings,

Andres Freund

Re: Missing dependency tracking for TableFunc nodes

[Mark Dilger]

On 11/11/19 1:41 PM, Tom Lane wrote:

I happened to notice that find_expr_references_walker has not
been taught anything about TableFunc nodes, which means it will
miss the type and collation OIDs embedded in such a node.

This can be demonstrated to be a problem by the attached script,
which will end up with a "cache lookup failed for type N"
error because we allow dropping a type the XMLTABLE construct
references.

This isn't hard to fix, as per the attached patch, but it makes
me nervous.  I wonder what other dependencies we might be missing.


I can consistently generate errors like the following in master:

  ERROR:  cache lookup failed for statistics object 31041

This happens in a stress test in which multiple processes are making 
changes to the schema.  So far, all the sessions that report this cache 
lookup error do so when performing one of ANALYZE, VACUUM ANALYZE, 
UPDATE, DELETE or EXPLAIN ANALYZE on a table that has MCV statistics. 
All processes running simultaneously are running the same set of 
functions, which create and delete tables, indexes, and statistics 
objects, insert, update, and delete rows in those tables, etc.


The fact that the statistics are of the MCV type might not be relevant; 
I'm creating those on tables as part of testing Tomas Vondra's MCV 
statistics patch, so all the tables have statistics of that kind on them.


I can try to distill my test case a bit, but first I'd like to know if 
you are interested.  Currently, the patch is over 2.2MB, gzip'd.  I'll 
only bother distilling it if you don't already know about these cache 
lookup failures.


--
Mark Dilger

Re: Role membership and DROP

[Tom Lane]

Laurenz Albe  writes:
> I realized only today that if role A is a member of role B,
> A can ALTER and DROP objects owned by B.
> I don't have a problem with that, but the documentation seems to
> suggest otherwise.  For example, for DROP TABLE:

>Only the table owner, the schema owner, and superuser can drop a table.

Generally, if you are a member of a role, that means you are the role for
privilege-test purposes.  I'm not on board with adding "(or a member of
that role)" to every place it could conceivably be added; I think that
would be more annoying than helpful.

It might be worth clarifying this point in section 5.7,

https://www.postgresql.org/docs/devel/ddl-priv.html

but let's not duplicate that in every ref/ page.

regards, tom lane

Re: Creating foreign key on partitioned table is too slow

[Alvaro Herrera]

On 2019-Nov-13, Alvaro Herrera wrote:

> On 2019-Nov-13, Tom Lane wrote:
> 
> > (BTW, a different question one could ask is exactly why
> > RelationBuildPartitionDesc is so profligate of leaked memory.)
> 
> The original partitioning code (f0e44751d717) decided that it didn't
> want to bother with adding a "free" routine for PartitionBoundInfo
> structs, maybe because it had too many pointers, so there's no way for
> RelationBuildPartitionDesc to free everything it allocates anyway.  We
> could add a couple of pfrees and list_frees here and there, but for the
> main thing being leaked we'd need to improve that API.

Ah, we also leak an array of PartitionBoundSpec, which is a Node.  Do we
have any way to free those?  I don't think we do.

In short, it looks to me as if this function was explicitly designed
with the idea that it'd be called in a temp mem context.

I looked at d3f48dfae42f again per your earlier suggestion.  Doing that
memory context dance for partitioned relations does seem to fix the
problem too; we just need to move the context creation to just after
ScanPgRelation, at which point we have the relkind.  (Note: I think the
problematic case is the partitioned table, not the partitions
themselves.  At least, with the attached patch the problem goes away.  I
guess it would be sensible to research whether we need to do this for
relispartition=true as well, but I haven't done that.)

There is indeed some leakage for relations that have triggers too (or
rules), but in order for those to become significant you would have to
have thousands of triggers or rules ...  and in reasonable designs, you
just don't because it doesn't make sense.  But it is not totally
unreasonable to have lots of partitions, and as we improve the system,
more and more people will want to.


Aside: while messing with this I noticed that how significant pg_strtok
is as a resource hog when building partition descs (from the
stringToNode that's applied to each partition's partbound.)

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index e8d11a1d0e..de6d6b3555 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -1029,28 +1029,8 @@ RelationBuildDesc(Oid targetRelId, bool insertIt)
 	Oid			relid;
 	HeapTuple	pg_class_tuple;
 	Form_pg_class relp;
-
-	/*
-	 * This function and its subroutines can allocate a good deal of transient
-	 * data in CurrentMemoryContext.  Traditionally we've just leaked that
-	 * data, reasoning that the caller's context is at worst of transaction
-	 * scope, and relcache loads shouldn't happen so often that it's essential
-	 * to recover transient data before end of statement/transaction.  However
-	 * that's definitely not true in clobber-cache test builds, and perhaps
-	 * it's not true in other cases.  If RECOVER_RELATION_BUILD_MEMORY is not
-	 * zero, arrange to allocate the junk in a temporary context that we'll
-	 * free before returning.  Make it a child of caller's context so that it
-	 * will get cleaned up appropriately if we error out partway through.
-	 */
-#if RECOVER_RELATION_BUILD_MEMORY
-	MemoryContext tmpcxt;
-	MemoryContext oldcxt;
-
-	tmpcxt = AllocSetContextCreate(CurrentMemoryContext,
-   "RelationBuildDesc workspace",
-   ALLOCSET_DEFAULT_SIZES);
-	oldcxt = MemoryContextSwitchTo(tmpcxt);
-#endif
+	MemoryContext tmpcxt = NULL;
+	MemoryContext oldcxt = NULL;
 
 	/*
 	 * find the tuple in pg_class corresponding to the given relation id
@@ -1061,14 +1041,7 @@ RelationBuildDesc(Oid targetRelId, bool insertIt)
 	 * if no such tuple exists, return NULL
 	 */
 	if (!HeapTupleIsValid(pg_class_tuple))
-	{
-#if RECOVER_RELATION_BUILD_MEMORY
-		/* Return to caller's context, and blow away the temporary context */
-		MemoryContextSwitchTo(oldcxt);
-		MemoryContextDelete(tmpcxt);
-#endif
 		return NULL;
-	}
 
 	/*
 	 * get information from the pg_class_tuple
@@ -1077,6 +1050,33 @@ RelationBuildDesc(Oid targetRelId, bool insertIt)
 	relid = relp->oid;
 	Assert(relid == targetRelId);
 
+	/*
+	 * This function and its subroutines can allocate a good deal of transient
+	 * data in CurrentMemoryContext.  Traditionally we've just leaked that
+	 * data, reasoning that the caller's context is at worst of transaction
+	 * scope, and relcache loads shouldn't happen so often that it's essential
+	 * to recover transient data before end of statement/transaction.  However
+	 * that's definitely not true in clobber-cache test builds, and perhaps
+	 * it's not true in other cases.  If RECOVER_RELATION_BUILD_MEMORY is not
+	 * zero, arrange to allocate the junk in a temporary context that we'll
+	 * free before returning.  Make it a child of caller's context so that it
+	 * will get cleaned up appropriately if we error out partway through.
+	 *
+	 * Partitioned tables are notoriously leaky 

Role membership and DROP

[Laurenz Albe]

I realized only today that if role A is a member of role B,
A can ALTER and DROP objects owned by B.

I don't have a problem with that, but the documentation seems to
suggest otherwise.  For example, for DROP TABLE:

   Only the table owner, the schema owner, and superuser can drop a table.

Should I compose a doc patch, or is that too much of a corner case
to mention?  I wanted to ask before I do the repetetive work.

Yours,
Laurenz Albe

Re: Building infrastructure for B-Tree deduplication that recognizes when opclass equality is also equivalence

[Peter Geoghegan]

On Mon, Oct 28, 2019 at 11:11 AM Anastasia Lubennikova
 wrote:
> At first I implemented bitwise as default, because it is more common .
> Though, I agree that it's essential to avoid false positives here.
> The new version of the patch is attached. I also updated pg_dump.
>
> A few more open questions:
> 1) How to handle contrib modules that create new opclasses?
> Since default is 'not bitwise' it means that various operator classes
> created in extensions
> such as bloom, btree_gin and others, won't be able to take advantage of
> various optimizations
> that require the opclass to be BITWISE.

What optimizations? Do we anticipate that other index AMs will benefit
from BITWISE-ness?

> 'v2-Opclass-bitwise-equality-0002' patch simply adds BITWISE keyword
> where necessary.
>
> 2) Whether we should provide ALTER OPERATOR CLASS SET BITWISE syntax?

I think that that's probably not desirable. There should at least be a
strong practical advantage if we go that way. This would mean ALTER
OPERATOR CLASS could change the "substance" of an opclass, which is
fundamentally different from what it can do already (it currently just
changes the owner, or the schema that it is stored in).

> 3) Current patch modifies regression test so that it checks CREATE
> OPCLASS BITWISE syntax.
> Is there anything else worth testing? As I see it, this patch is just
> about infrastructure changes,
> and more specific tests will be added by features that will implement
> further optimizations.

I think so too -- this is really about associating a single piece of
information with an operator class.

BTW: No need to bump catversion when posting a patch, which I see in
"v2-Opclass-*0001.patch". That is our policy. (A catversion bump is
generally supposed to be done at the last minute, just as the patch is
committed. This avoids unnecessary conflicts against the master branch
over time, as a patch is developed.)

--
Peter Geoghegan

Re: [PATCH] gcc warning 'expression which evaluates to zero treated as a null pointer'

[Tom Lane]

didier  writes:
> clang -E output before 7a0574b5
> HeapTuple newtuple = 0;
> with 7a0574b5
> HeapTuple newtuple = ((bool) 0);

Hm, did you re-run configure after 7a0574b5?  If you didn't, it would
have gone through the not-stdbool.h path in c.h, which might account
for this.  It's a good catch though, even if by accident :-)

regards, tom lane

Re: [PATCH] gcc warning 'expression which evaluates to zero treated as a null pointer'

[didier]

Hi,
On Wed, Nov 13, 2019 at 8:52 PM Tom Lane  wrote:
>
> didier  writes:
> > Trivial patch:
> > - remove a gcc warning (since commit 7a0574b5)
> > expression which evaluates to zero treated as a null pointer constant of
> >   type 'HeapTuple' (aka 'struct HeapTupleData *')
>
> Hmm, the initializations "HeapTuple newtuple = false" are certainly
> bogus-looking and not per project style; I wonder who's to blame for
> those?  (I do not see what 7a0574b5 would have had to do with it;
> that didn't affect any backend code.)

My mistake it's not gcc but clang for JIT, maybe because it could
change false definition?
clang version: 6.0.0-1ubuntu2
clang -E output before 7a0574b5
HeapTuple newtuple = 0;
with 7a0574b5
HeapTuple newtuple = ((bool) 0);

>
> > - always use "if (newtuple == NULL)" rather than mixing !newtuple and
> > newtuple == NULL
>
> Don't particularly agree with these changes though.  "if (!ptr)" is
> a very common C idiom, and no programmer would tolerate a compiler
> that warned about it.
There's no warning, it's stylistic. In the same function there's both
forms a couple of lines apart: "if (!ptr)" follow by "if (ptr ==
NULL)", using only one form is smother on the brain, at least mine.

Regards
Didier

Re: ssl passphrase callback

[Tomas Vondra]

On Wed, Nov 13, 2019 at 02:48:01PM -0500, Andrew Dunstan wrote:


On 11/13/19 8:08 AM, Bruce Momjian wrote:




Also, why was this patch posted without any discussion of these issues?
Shouldn't we ideally discuss the API first?



This is a very tiny patch. I don't think it's unusual to post such
things without prior discussion. I would agree with your point if it
were thousands of lines instead of 20 or so lines of core code.



Not sure that's really true. I think patches should provide some basic
explanation why the feature is desirable, no matter the number of lines.

Also, there were vague references to issues with passing parameters to
archive_command. A link to details, past discussion, or brief
explanation would be nice.


regards

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

Re: ssl passphrase callback

[Tomas Vondra]

On Wed, Nov 13, 2019 at 01:20:43PM +, Simon Riggs wrote:

On Wed, 13 Nov 2019 at 13:08, Bruce Momjian  wrote:


On Tue, Nov 12, 2019 at 09:51:33PM -0500, Bruce Momjian wrote:
> On Sun, Nov 10, 2019 at 01:01:17PM -0600, Magnus Hagander wrote:
> > On Wed, Nov 6, 2019 at 7:24 PM Bruce Momjian  wrote:

> > One of the main reasons there being to be easily able to transfer more
state
> > and give results other than just an exit code, no need to deal with
parameter
> > escaping etc. Which probably wouldn't matter as much to an SSL
passphrase
> > command, but still.
>
> I get the callback-is-easier issue with shared objects, but are we
> expecting to pass in more information here than we do for
> archive_command?  I would think not.  What I am saying is that if we
> don't think passing things in works, we should fix all these external
> commands, or something.   I don't see why ssl_passphrase_command is
> different, except that it is new.





Or is it related to _securely_passing something?



Yes



I think it would be beneficial to explain why shared object is more
secure than an OS command. Perhaps it's common knowledge, but it's not
quite obvious to me.




> Also, why was this patch posted without any discussion of these issues?
> Shouldn't we ideally discuss the API first?

I wonder if every GUC that takes an OS command should allow a shared
object to be specified --- maybe control that if the command string
starts with a # or something.



Very good idea



If it's about securely passing sensitive information (i.e. passphrase)
as was suggested, then I think that only applies to fairly small number
of GUCs.


regards

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

Re: JIT performance bug/regression & JIT EXPLAIN

[Andres Freund]

Hi,

On 2019-11-13 14:29:07 -0500, Robert Haas wrote:
> On Mon, Oct 28, 2019 at 7:21 PM Andres Freund  wrote:
> > Because that's the normal way to represent something non-existing for
> > formats like json? There's a lot of information we show always for !text
> > format, even if not really applicable to the context (e.g. Triggers for
> > select statements). I think there's an argument to made to deviate in
> > this case, but I don't think it's obvious.
> 
> I've consistently been of the view that anyone who thinks that the
> FORMAT option should affect what information gets displayed doesn't
> understand the meaning of the word "format." And I still feel that
> way.

Well, it's not been that way since the format option was added, so ...



> I also think that conditionally renaming "Output" to "Project" is a
> super-bad idea. The idea of a format like this is that the "keys" stay
> constant and the values change. If you need to tell people more, you
> add more keys.

Yea, I don't like the compat break either.  But I'm not so convinced
that just continuing to collect cruft because of compatibility is worth
it - I just don't see an all that high reliance interest for explain
output.

I think adding a new key is somewhat ok for !text, but for text that
doesn't seem like an easy solution?

I kind of like my idea somewhere downthread, in a reply to Maciek, of
simply not listing "Output" for nodes that don't project.  While that's
still a format break, it seems that tools already need to deal with
"Output" not being present?


> I also think that making the Filter field a group conditionally is a
> bad idea, for similar reasons.

Oh, yea, it's utterly terrible (I called it crappy in my email :)).


> But making it always be a group doesn't necessarily seem like a bad
> idea. I think, though, that you could handle this in other ways, like
> by suffixing existing keys.  e.g. if you've got Index-Qual and Filter,
> just do Index-Qual-JIT and Filter-JIT and call it good.

Maciek suggested the same. But to me it seems going down that way will
make the format harder and harder to understand? So I think I'd rather
break compat here, and go for a group.

Personally I think the group naming choice for explain makes the the
!text outputs much less useful than they could be - we basically force
every tool to understand all possible keys, to make sense of formatted
output. Instead of something like 'Filter: {"Qual":{"text" : "...",
"JIT": ...}' where a tool only needed to understand that everything that
has a "Qual" inside is a filtering expression, everything that has a
"Project" is a projecting type of expression, ... a tool needs to know
about "Inner Cond", "Order By", "Filter", "Recheck Cond", "TID Cond",
"Join Filter", "Merge Cond", "Hash Cond", "One-Time Filter", ...

Greetings,

Andres Freund

Re: Creating foreign key on partitioned table is too slow

[Alvaro Herrera]

On 2019-Nov-13, Tom Lane wrote:

> (BTW, a different question one could ask is exactly why
> RelationBuildPartitionDesc is so profligate of leaked memory.)

The original partitioning code (f0e44751d717) decided that it didn't
want to bother with adding a "free" routine for PartitionBoundInfo
structs, maybe because it had too many pointers, so there's no way for
RelationBuildPartitionDesc to free everything it allocates anyway.  We
could add a couple of pfrees and list_frees here and there, but for the
main thing being leaked we'd need to improve that API.

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

Re: Invisible PROMPT2

[David Fetter]

On Wed, Nov 13, 2019 at 03:58:38PM -0300, Alvaro Herrera wrote:
> On 2019-Nov-13, David Fetter wrote:
> 
> > On Wed, Nov 13, 2019 at 03:06:08PM -0300, Alvaro Herrera wrote:
> > > On 2019-Nov-13, David Fetter wrote:
> > > 
> > > > On Wed, Nov 13, 2019 at 09:47:01AM -0500, Tom Lane wrote:
> > > 
> > > > > > How about a circumfix directive (like the existing %[ ... %])
> > > > > > that replaces everything inside with whitespace, but keeps the 
> > > > > > width?
> 
> > > This seems way too specific to me.  I like the "circumfix" directive
> > > better, because it allows one to do more things.  I don't have any
> > > immediate use for it, but it doesn't seem completely far-fetched that
> > > there are some.
> 
> > So something like %w[...%w] where people could put things like PROMPT1
> > inside?
> 
> Hmm, (I'm not sure your proposed syntax works, but let's assume that
> it does.)  I'm saying you'd define
> \set PROMPT1 '%a%b%c '
> \set PROMPT2 '%w[%a%b%c %w]'
> 
> and you'd end up with matching indentation on multiline queries.
> 
> I'm not sure that we'd need to make something like this work:
>   PROMPT1="%w[$PROMPT1%w]"
> which I think is what you're saying.

PROMPT2="%w[$PROMPT1%w]", and basically yes.

> We already have "%:PROMPT1:" but that expands to the literal value of
> prompt1, not to the value that prompt1 would expand to:

Yeah, that's not so great for this usage.  I guess "expand variables"
could be a separate useful feature (and patch) all by itself...

Best,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

Re: [PATCH] gcc warning 'expression which evaluates to zero treated as a null pointer'

[Tom Lane]

didier  writes:
> Trivial patch:
> - remove a gcc warning (since commit 7a0574b5)
> expression which evaluates to zero treated as a null pointer constant of
>   type 'HeapTuple' (aka 'struct HeapTupleData *')

Hmm, the initializations "HeapTuple newtuple = false" are certainly
bogus-looking and not per project style; I wonder who's to blame for
those?  (I do not see what 7a0574b5 would have had to do with it;
that didn't affect any backend code.)

> - always use "if (newtuple == NULL)" rather than mixing !newtuple and
> newtuple == NULL

Don't particularly agree with these changes though.  "if (!ptr)" is
a very common C idiom, and no programmer would tolerate a compiler
that warned about it.

regards, tom lane

Re: [HACKERS] [WIP] Effective storage of duplicates in B-tree index.

[Peter Geoghegan]

On Wed, Nov 13, 2019 at 11:33 AM Robert Haas  wrote:
> On Tue, Nov 12, 2019 at 6:22 PM Peter Geoghegan  wrote:
> > * Disabled deduplication in system catalog indexes by deeming it
> > generally unsafe.
>
> I (continue to) think that deduplication is a terrible name, because
> you're not getting rid of the duplicates. You are using a compressed
> representation of the duplicates.

"Deduplication" never means that you get rid of duplicates. According
to Wikipedia's deduplication article: "Whereas compression algorithms
identify redundant data inside individual files and encodes this
redundant data more efficiently, the intent of deduplication is to
inspect large volumes of data and identify large sections – such as
entire files or large sections of files – that are identical, and
replace them with a shared copy".

This seemed like it fit what this patch does. We're concerned with a
specific, simple kind of redundancy. Also:

* From the user's point of view, we're merging together what they'd
call duplicates. They don't really think of the heap TID as part of
the key.

* The term "compression" suggests a decompression penalty when
reading, which is not the case here.

* The term "compression" confuses the feature added by the patch with
TOAST compression. Now we may have two very different varieties of
compression in the same index.

Can you suggest an alternative?

-- 
Peter Geoghegan

Re: ssl passphrase callback

[Andrew Dunstan]

On 11/13/19 8:08 AM, Bruce Momjian wrote:
>
>>
>> Also, why was this patch posted without any discussion of these issues?
>> Shouldn't we ideally discuss the API first?


This is a very tiny patch. I don't think it's unusual to post such
things without prior discussion. I would agree with your point if it
were thousands of lines instead of 20 or so lines of core code.


cheers


andrew


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

Re: global / super barriers (for checksums)

[Andres Freund]

Hi,

On 2019-11-13 12:26:34 -0500, Robert Haas wrote:
> TL;DR: I'm not sure that we need 0001; I propose to commit 0002; and I
> have some concerns about 0003 and am interested in working further on
> it.

Thanks for looking at the patch!


> 0001 changes the StartBackgroundWorker so that the SIGINT handler is
> contingent on BGWORKER_SHMEM_ACCESS rather than
> BGWORKER_BACKEND_DATABASE_CONNECTION. It seems to me that the goal
> here should be to use procsignal_sigusr1_handler(), or something that
> calls it, in any process where ProcSignalInit() is called, but a
> backend that only requests BGWORKER_SHMEM_ACCESS probably won't,
> because the normal way for a background worker to call
> ProcSignalInit() would be to indirectly call InitPostgres() by means
> of BackgroundWorkerInitializeConnection() or
> BackgroundWorkerInitializeConnectionByOid(), and a worker that didn't
> ask for a database connection presumably shouldn't be doing that. So
> I'm not sure that I understand why we need this. (Is it legal for a
> worker to call one of these functions as long as it passes InvalidOid
> for the database OID, or something? Do we have examples of workers
> that do that?)

Hm. Well, it seems useful for non-database connected processes to be
able to partake in global barriers. Without that, if there's any chance
they could e.g. generate WAL, it'd e.g. break the checksum enablement
patch. Note that auxiliary processes already do call ProcSignalInit().

You're right that we ought to make it easier (or automatic) to call
ProcSignalInit() for such processes.  Perhaps we ought to do so in
ProcessInit()?

But perhaps we don't strictly need this - I'm not sure how many examples
of BGWORKER_SHMEM_ACCESS bgworkers that don't also use
BGWORKER_BACKEND_DATABASE_CONNECTION there are.



> Regarding 0003:
> 
> - The proc->pid == 0 check in EmitGlobalBarrier() doesn't really do
> what it appears to do, because regular backends don't clear proc->pid
> when they exit; only auxiliary processes do. (We could, and perhaps
> should, change that.)

Ick.


> - It seems  to me that it would be generally better to insert
> CHECK_FOR_INTERRUPTS() in places where the patch just inserts an
> ad-hoc if (GlobalBarrierInterruptPending)
> ProcessGlobalBarrierIntterupt(). There might be someplace where we
> have to do it the latter way because we unavoidably hold an LWLock or
> something, but I think we should avoid that whenever possible. If it's
> a good place to check for one kind of interrupt, it's probably a good
> place to check for all of them.

I might be missing something. Aren't all of the places where those
checks are places where we currently can't do a CHECK_FOR_INTERRUPTS()?
I've swapped this thoroughly out of my mind, but going through them:

1) AutoVacLauncherMain() - doesn't do CFI()
2) BackgroundWriterMain() - dito
3) CheckpointerMain() - dito
4) HandleStartupProcInterrupts() - dito
5) WalWriterMain() - dito
6) BufferSync() - dito, called from CheckpointerMain(), and startup process
7) ProcessClientWriteInterrupt() - can't do generic CFI, don't want to
   process all interrupts while writing out data, to avoid corrupting
   the output stream or loosing messages

Which one do you think we should convert to CFI()? As far as I can tell
we can't make the non-backend cases use the postgres.c
ProcessInterrupts(), and the ProcessClientWriteInterrupt() one can't do
so either.


> - I don't think BufferSync() is doing this in the right place. Unless
> I am confused, it's doing it inside a loop that just allocates stuff
> and copies data around, which probably does not need this kind of
> decoration. It wouldn't hurt to check here, and it might be a good
> idea for safety, but I think the place that really needs this
> treatment is the following loop where we are actually doing I/O and
> sleeping. Possibly we should even be doctoring CheckpointWriteDelay to
> use a latch-wait loop that can CHECK_FOR_INTERRUPTS() before
> re-sleeping, although for 100ms (curiously described as a Big Sleep)
> it might be overkill.

Yea. Not sure what happened there. I think it's good to have such a
check in all the buffer loops in BufferSync(), but clearly it's most
important to have one in the write case.


> - I think it would be nice to make this system more extensible. IIUC,
> the idea is that ProcessGlobalBarrierIntterupt() will call bespoke
> code for each GLOBBAR_* flag that set, and that code will then update
> process-local state from global state before returning. But, instead
> of hard-coding constants, how about just having a list of callbacks,
> and we call them all here, and each one is responsible for figuring
> out whether anything's been changed that it cares about, and if so
> updating the appropriate local state?

I don't think that's a good idea. This stuff happens at an extremely low
level, in different types of processes, with various locks held
etc. Running arbitrary code in those circumstances strikes me as a
seriously bad idea.


> 

Re: [HACKERS] [WIP] Effective storage of duplicates in B-tree index.

[Robert Haas]

On Tue, Nov 12, 2019 at 6:22 PM Peter Geoghegan  wrote:
> * Disabled deduplication in system catalog indexes by deeming it
> generally unsafe.

I (continue to) think that deduplication is a terrible name, because
you're not getting rid of the duplicates. You are using a compressed
representation of the duplicates.

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

Re: Creating foreign key on partitioned table is too slow

[Tom Lane]

Alvaro Herrera  writes:
> On 2019-Oct-25, Tomas Vondra wrote:
>> The attached patch trivially fixes that by adding a memory context
>> tracking all the temporary data, and then just deletes it as a whole at
>> the end of the function. This significantly reduces the memory usage for
>> me, not sure it's 100% correct.

> FWIW we already had this code (added by commit 2455ab48844c), but it was
> removed by commit d3f48dfae42f.  I think we should put it back.

I disagree.  The point of d3f48dfae42f is that the management of that
leakage is now being done at the caller level, and I'm quite firmly
against having RelationBuildPartitionDesc duplicate that.  If we
don't like the amount of space RelationBuildPartitionDesc is leaking,
we aren't going to like the amount of space that sibling routines
such as RelationBuildTriggers leak, either.

What we ought to be thinking about instead is adjusting the
RECOVER_RELATION_BUILD_MEMORY heuristic in relcache.c.  I am not
sure what it ought to look like, but I doubt that "do it all the
time" has suddenly become the right answer, when it wasn't the
right answer for 20-something years.

It's conceivable that "do it if CCA is on, or if the current
table is a partition child table" is a reasonable approach.
But I'm not sure whether we can know the relation relkind
early enough for that :-(

(BTW, a different question one could ask is exactly why
RelationBuildPartitionDesc is so profligate of leaked memory.)

regards, tom lane

Re: JIT performance bug/regression & JIT EXPLAIN

[Robert Haas]

On Mon, Oct 28, 2019 at 7:21 PM Andres Freund  wrote:
> Because that's the normal way to represent something non-existing for
> formats like json? There's a lot of information we show always for !text
> format, even if not really applicable to the context (e.g. Triggers for
> select statements). I think there's an argument to made to deviate in
> this case, but I don't think it's obvious.

I've consistently been of the view that anyone who thinks that the
FORMAT option should affect what information gets displayed doesn't
understand the meaning of the word "format." And I still feel that
way.

I also think that conditionally renaming "Output" to "Project" is a
super-bad idea. The idea of a format like this is that the "keys" stay
constant and the values change. If you need to tell people more, you
add more keys.

I also think that making the Filter field a group conditionally is a
bad idea, for similar reasons. But making it always be a group doesn't
necessarily seem like a bad idea. I think, though, that you could
handle this in other ways, like by suffixing existing keys.  e.g. if
you've got Index-Qual and Filter, just do Index-Qual-JIT and
Filter-JIT and call it good.

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

Re: AtEOXact_Snapshot timing

[Robert Haas]

On Mon, Nov 11, 2019 at 2:12 PM Andres Freund  wrote:
> Hm. I feel like there's plenty reasons to get a snapshot in extensions -
> there's plenty APIs one cannot really call without doing so?

Sure, I don't disagree with that.

> What I'm
> worried about is not primarily that GetSnapshotData() is being called
> directly, but that $author got a snapshot previously, and then tries to
> use it in an xact callback or such.

Yeah, I guess that's possible. Registering a new snapshot would trip
one of the assertions I added, but using an old one wouldn't.

> I'd add asserts to at least PushActiveSnapshot(), and I don't see the
> harm in adding one to GetSnapshotData().

OK. I think there's some risk that someone will have a purpose for
calling GetSansphotData() which is actually tolerably safe but now
won't work, so I thin there's a chance we'll get a complaint. But if
we do, we can always reconsider whether to take that particular
assertion back out again.

> Based on what I've seen people do in xact callback handlers... Didn't PG
> itself e.g. have code doing syscache lookups in aborted transactions a
> couple times?

Sure -- but the assertions I had already added would catch that
anyway, assuming that it actually attempted catalog access, and we now
also have assertions that will catch it even if it would have been
satisfied from the cache.

> The danger of using a previously acquired snapshot in that stage is why
> I was pondering adding an assert to visibility functions
> themselves. E.g. just adding one to HeapTupleSatisfiesVisibility() might
> already add a good bit of coverage.

Yeah, but I really hate to do that; those functions are super-hot. And
I don't think we need to go overboard in protecting people from
themselves. The assertions I'm proposing to add should already catch
quite a bit of stuff that is unchecked today, and with little or no
possible downside. There's no rule that we can't add more later, nor
are more assertions always better than fewer.

> > I agree with you that it would be nicer if we could put the killing of
> > the old snapshots directly adjacent to ProcArrayEndTransaction(), and
> > that was the first thing I tried, but it doesn't work, because
> > resource owner cleanup has to run first.
>
> Hm. I'd even say that it actually belongs to *before* the
> ProcArrayEndTransaction() call.
>
> For a bit I wondered if the resowner snapshot cleanup ought to be at
> least moved to RESOURCE_RELEASE_BEFORE_LOCKS. Not that it actually
> addresses this issue, but it seems to belong there "thematically". But
> then I honestly don't understand why most of the resowner managed
> resources in the abort sequence are released where they are.  The only
> really explanatory comment is:
>
>  * The ordering of operations is not entirely random.  The idea is:
>  * release resources visible to other backends (eg, files, buffer 
> pins);
>  * then release locks; then release backend-local resources. We want 
> to
>  * release locks at the point where any backend waiting for us will 
> see
>  * our transaction as being fully cleaned up.
>
> but that doesn't explain why we e.g. process relcache references, jit
> contexts (arguably it does for dsm segments), at that stage. And
> definitely not why we do abort's relcache inval processing between
> RESOURCE_RELEASE_BEFORE_LOCKS and RESOURCE_RELEASE_LOCKS - that can be
> quite expensive whe needing to scan the whole relcache.
>
> Anyway, this is grumbling about things far beyond the scope of this
> patch.

Yeah. I do agree with you that a lot of that stuff isn't very well
explained.  Nor is there much of an explanation of why some things go
through resowner.c and other things have bespoke cleanup code. But, as
you say, that's out of scope.

> I'm not sure that's true - I've certainly seen extensions logging the
> transaction state into a table, for example... Even in aborted xacts.

Whoa.

> > Non-catalog access would probably fail the assertion in
> > GetTransactionSnapshot() or GetLatestSnapshot().
>
> Not this patch's fault, obviously, but none of this appears to catch
> DML...

Really? It's hard to imagine that DML wouldn't attempt catalog access.

> Just to be clear: While I think an assert or two more seem like a good
> idea, that's musing around the edges, not a fundamental concern.

All right, here's another version with an assert or two more. :-)

> > diff --git a/src/backend/access/transam/xact.c 
> > b/src/backend/access/transam/xact.c
> > index f594d33e7a..9993251607 100644
> > --- a/src/backend/access/transam/xact.c
> > +++ b/src/backend/access/transam/xact.c
> > @@ -2732,6 +2732,18 @@ AbortTransaction(void)
> >   pgstat_report_xact_timestamp(0);
> >   }
> >
> > + /*
> > +  * Any snapshots taken by this transaction were unsafe to use even at 
> > the
> > +  * time when we entered AbortTransaction(), since we might have, for
> > +  * example, inserted a heap tuple and failed 

Re: Invisible PROMPT2

[Alvaro Herrera]

On 2019-Nov-13, David Fetter wrote:

> On Wed, Nov 13, 2019 at 03:06:08PM -0300, Alvaro Herrera wrote:
> > On 2019-Nov-13, David Fetter wrote:
> > 
> > > On Wed, Nov 13, 2019 at 09:47:01AM -0500, Tom Lane wrote:
> > 
> > > > > How about a circumfix directive (like the existing %[ ... %])
> > > > > that replaces everything inside with whitespace, but keeps the width?

> > This seems way too specific to me.  I like the "circumfix" directive
> > better, because it allows one to do more things.  I don't have any
> > immediate use for it, but it doesn't seem completely far-fetched that
> > there are some.

> So something like %w[...%w] where people could put things like PROMPT1
> inside?

Hmm, (I'm not sure your proposed syntax works, but let's assume that
it does.)  I'm saying you'd define
\set PROMPT1 '%a%b%c '
\set PROMPT2 '%w[%a%b%c %w]'

and you'd end up with matching indentation on multiline queries.

I'm not sure that we'd need to make something like this work:
  PROMPT1="%w[$PROMPT1%w]"
which I think is what you're saying.


We already have "%:PROMPT1:" but that expands to the literal value of
prompt1, not to the value that prompt1 would expand to:

55432 13devel 11214=# \set PROMPT2 'hello %:PROMPT1: bye'
55432 13devel 11214=# select
hello %[%033[35m%]%> %:VERSION_NAME: %p%[%033[0m%]%R%#  bye

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

Re: Creating foreign key on partitioned table is too slow

[Alvaro Herrera]

On 2019-Oct-25, Tomas Vondra wrote:

> The attached patch trivially fixes that by adding a memory context
> tracking all the temporary data, and then just deletes it as a whole at
> the end of the function. This significantly reduces the memory usage for
> me, not sure it's 100% correct.

FWIW we already had this code (added by commit 2455ab48844c), but it was
removed by commit d3f48dfae42f.  I think we should put it back.  (I
think it may be useful to use a static MemoryContext that we can just
reset each time, instead of creating and deleting each time, to save on
memcxt churn.  That'd make the function non-reentrant, but I don't see
that we'd make the catalogs partitioned any time soon.  This may be
premature optimization though -- not really wedded to it.)

With Amit's patch to make RelationBuildPartitionDesc called lazily, the
time to plan the RI_InitialCheck query (using Kato Sho's test case) goes
from 30 seconds to 14 seconds on my laptop.  Obviously there's more that
we'd need to fix to make the scenario fully supported, but it seems a
decent step forward.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>From c9000408036903df95ae22d0918a3fffddc7bf48 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera 
Date: Wed, 13 Nov 2019 15:12:05 -0300
Subject: [PATCH 1/2] build partdesc memcxt

---
 src/backend/partitioning/partdesc.c | 15 +--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/src/backend/partitioning/partdesc.c b/src/backend/partitioning/partdesc.c
index 6ede084afe..c551df6673 100644
--- a/src/backend/partitioning/partdesc.c
+++ b/src/backend/partitioning/partdesc.c
@@ -71,6 +71,14 @@ RelationBuildPartitionDesc(Relation rel)
 	PartitionKey key = RelationGetPartitionKey(rel);
 	MemoryContext oldcxt;
 	int		   *mapping;
+	static MemoryContext tmpContext = NULL;
+
+	if (tmpContext == NULL)
+		tmpContext = AllocSetContextCreate(TopMemoryContext,
+		   "build part tmp",
+		   ALLOCSET_SMALL_SIZES);
+
+	oldcxt = MemoryContextSwitchTo(tmpContext);
 
 	/*
 	 * Get partition oids from pg_inherits.  This uses a single snapshot to
@@ -205,11 +213,11 @@ RelationBuildPartitionDesc(Relation rel)
 		boundinfo = partition_bounds_create(boundspecs, nparts, key, );
 
 		/* Now copy all info into relcache's partdesc. */
-		oldcxt = MemoryContextSwitchTo(rel->rd_pdcxt);
+		MemoryContextSwitchTo(rel->rd_pdcxt);
 		partdesc->boundinfo = partition_bounds_copy(boundinfo, key);
 		partdesc->oids = (Oid *) palloc(nparts * sizeof(Oid));
 		partdesc->is_leaf = (bool *) palloc(nparts * sizeof(bool));
-		MemoryContextSwitchTo(oldcxt);
+		MemoryContextSwitchTo(tmpContext);
 
 		/*
 		 * Assign OIDs from the original array into mapped indexes of the
@@ -231,6 +239,9 @@ RelationBuildPartitionDesc(Relation rel)
 		}
 	}
 
+	MemoryContextSwitchTo(oldcxt);
+	MemoryContextResetAndDeleteChildren(tmpContext);
+
 	rel->rd_partdesc = partdesc;
 }
 
-- 
2.20.1

>From 6deb1e88a188046939c8706eff8ad3e9f3121298 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera 
Date: Wed, 13 Nov 2019 15:34:13 -0300
Subject: [PATCH 2/2] Invoke RelationBuildPartitionDesc lazily

---
 src/backend/partitioning/partdesc.c | 17 -
 src/backend/utils/cache/relcache.c  | 22 +-
 src/include/partitioning/partdesc.h |  2 +-
 src/include/utils/rel.h |  6 --
 4 files changed, 22 insertions(+), 25 deletions(-)

diff --git a/src/backend/partitioning/partdesc.c b/src/backend/partitioning/partdesc.c
index c551df6673..8171eb03ca 100644
--- a/src/backend/partitioning/partdesc.c
+++ b/src/backend/partitioning/partdesc.c
@@ -34,6 +34,7 @@
 #include "utils/rel.h"
 #include "utils/syscache.h"
 
+
 typedef struct PartitionDirectoryData
 {
 	MemoryContext pdir_mcxt;
@@ -47,6 +48,20 @@ typedef struct PartitionDirectoryEntry
 	PartitionDesc pd;
 } PartitionDirectoryEntry;
 
+
+static void RelationBuildPartitionDesc(Relation rel);
+
+PartitionDesc
+RelationGetPartitionDesc(Relation rel)
+{
+	Assert(rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE);
+
+	if (rel->rd_partdesc == NULL)
+		RelationBuildPartitionDesc(rel);
+
+	return rel->rd_partdesc;
+}
+
 /*
  * RelationBuildPartitionDesc
  *		Form rel's partition descriptor, and store in relcache entry
@@ -57,7 +72,7 @@ typedef struct PartitionDirectoryEntry
  * that's sufficient to prevent that can assume that rd_partdesc
  * won't change underneath it.
  */
-void
+static void
 RelationBuildPartitionDesc(Relation rel)
 {
 	PartitionDesc partdesc;
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index ad1ff01b32..5ef61d187a 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -1165,20 +1165,17 @@ RelationBuildDesc(Oid targetRelId, bool insertIt)
 	relation->rd_fkeylist = NIL;
 	relation->rd_fkeyvalid = false;
 
-	/* if a partitioned table, initialize key and partition descriptor info */
+	/* if 

Re: Invisible PROMPT2

[David Fetter]

On Wed, Nov 13, 2019 at 03:06:08PM -0300, Alvaro Herrera wrote:
> On 2019-Nov-13, David Fetter wrote:
> 
> > On Wed, Nov 13, 2019 at 09:47:01AM -0500, Tom Lane wrote:
> 
> > > > How about a circumfix directive (like the existing %[ ... %])
> > > > that replaces everything inside with whitespace, but keeps the width?
> > > 
> > > Or just define %w as meaning "whitespace of the same width as
> > > PROMPT1".  You couldn't use it *in* PROMPT1, then, but I see
> > > no use-case for that anyway.
> > 
> > +1 for doing it this way.  Would it make more sense to error out if
> > somebody tried to set that in PROMPT1, or ignore it, or...?
> 
> This seems way too specific to me.  I like the "circumfix" directive
> better, because it allows one to do more things.  I don't have any
> immediate use for it, but it doesn't seem completely far-fetched that
> there are some.
> 
> BTW the psql manual says that %[ and %] were plagiarized from tcsh, but
> that's a lie: tcsh does not contain such a feature.  Bash does, however.
> (I guess not many people read the tcsh manual.)
> 
> Neither bash nor tcsh have a feature to return whitespace of anything;
> we're in a green field here ISTM.

So something like %w[...%w] where people could put things like PROMPT1
inside?

Best,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

Re: Invisible PROMPT2

[Alvaro Herrera]

On 2019-Nov-13, David Fetter wrote:

> On Wed, Nov 13, 2019 at 09:47:01AM -0500, Tom Lane wrote:

> > > How about a circumfix directive (like the existing %[ ... %])
> > > that replaces everything inside with whitespace, but keeps the width?
> > 
> > Or just define %w as meaning "whitespace of the same width as
> > PROMPT1".  You couldn't use it *in* PROMPT1, then, but I see
> > no use-case for that anyway.
> 
> +1 for doing it this way.  Would it make more sense to error out if
> somebody tried to set that in PROMPT1, or ignore it, or...?

This seems way too specific to me.  I like the "circumfix" directive
better, because it allows one to do more things.  I don't have any
immediate use for it, but it doesn't seem completely far-fetched that
there are some.

BTW the psql manual says that %[ and %] were plagiarized from tcsh, but
that's a lie: tcsh does not contain such a feature.  Bash does, however.
(I guess not many people read the tcsh manual.)

Neither bash nor tcsh have a feature to return whitespace of anything;
we're in a green field here ISTM.

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

Re: Using multiple extended statistics for estimates

[Mark Dilger]

On 11/13/19 7:28 AM, Tomas Vondra wrote:

Hi,

here's an updated patch, with some minor tweaks based on the review and
added tests (I ended up reworking those a bit, to make them more like
the existing ones).


Thanks, Tomas, for the new patch set!

Attached are my review comments so far, in the form of a patch applied 
on top of yours.


--
Mark Dilger
diff --git a/src/backend/statistics/dependencies.c b/src/backend/statistics/dependencies.c
index a6ca11c675..ccf6e41b9c 100644
--- a/src/backend/statistics/dependencies.c
+++ b/src/backend/statistics/dependencies.c
@@ -850,7 +850,8 @@ dependency_is_compatible_clause(Node *clause, Index relid, AttrNumber *attnum)
  *		find the strongest dependency on the attributes
  *
  * When applying functional dependencies, we start with the strongest
- * dependencies. That is, we select the dependency that:
+ * dependencies. That is, we select the best dependency in the following
+ * descending order of preference:
  *
  * (a) has all attributes covered by equality clauses
  *
@@ -860,6 +861,9 @@ dependency_is_compatible_clause(Node *clause, Index relid, AttrNumber *attnum)
  *
  * This guarantees that we eliminate the most redundant conditions first
  * (see the comment in dependencies_clauselist_selectivity).
+ *
+ * TODO: Rename 'dependencies', as that name is what you'd expect for an
+ *   argument of type (MVDependencies *), not (MVDependencies **)
  */
 static MVDependency *
 find_strongest_dependency(MVDependencies **dependencies, int ndependencies,
@@ -897,11 +901,14 @@ find_strongest_dependency(MVDependencies **dependencies, int ndependencies,
 
 /* also skip weaker dependencies when attribute count matches */
 if (strongest->nattributes == dependency->nattributes &&
-	strongest->degree > dependency->degree)
+	strongest->degree > dependency->degree)		/* TODO: Why not ">=" here? */
 	continue;
 			}
 
 			/*
+			 * TODO: As coded, this dependency is "at least as strong as", not
+			 * necessarily "stronger".
+			 *
 			 * this dependency is stronger, but we must still check that it's
 			 * fully matched to these attnums. We perform this check last as it's
 			 * slightly more expensive than the previous checks.
@@ -942,7 +949,7 @@ find_strongest_dependency(MVDependencies **dependencies, int ndependencies,
  */
 Selectivity
 dependencies_clauselist_selectivity(PlannerInfo *root,
-	List *clauses,
+	const List *clauses,
 	int varRelid,
 	JoinType jointype,
 	SpecialJoinInfo *sjinfo,
@@ -1084,6 +1091,11 @@ dependencies_clauselist_selectivity(PlannerInfo *root,
 			 * to a different Const in another clause then no rows will match
 			 * anyway. If it happens to be compared to the same Const, then
 			 * ignoring the additional clause is just the thing to do.
+			 *
+			 * TODO: Improve this code comment.  Specifically, why would we
+			 * ignore that no rows will match?  It seems that such a discovery
+			 * would allow us to return an estimate of 0 rows, and that would
+			 * be useful.
 			 */
 			if (dependency_implies_attribute(dependency,
 			 list_attnums[listidx]))
diff --git a/src/backend/statistics/extended_stats.c b/src/backend/statistics/extended_stats.c
index b55799b8b1..a1798a6b91 100644
--- a/src/backend/statistics/extended_stats.c
+++ b/src/backend/statistics/extended_stats.c
@@ -1202,7 +1202,7 @@ statext_mcv_remaining_attnums(int nclauses, Bitmapset **estimatedclauses,
  * already have a bit set.
  */
 static Selectivity
-statext_mcv_clauselist_selectivity(PlannerInfo *root, List *clauses, int varRelid,
+statext_mcv_clauselist_selectivity(PlannerInfo *root, const List *clauses, int varRelid,
    JoinType jointype, SpecialJoinInfo *sjinfo,
    RelOptInfo *rel, Bitmapset **estimatedclauses)
 {
@@ -1340,7 +1340,7 @@ statext_mcv_clauselist_selectivity(PlannerInfo *root, List *clauses, int varReli
  *		Estimate clauses using the best multi-column statistics.
  */
 Selectivity
-statext_clauselist_selectivity(PlannerInfo *root, List *clauses, int varRelid,
+statext_clauselist_selectivity(PlannerInfo *root, const List *clauses, int varRelid,
 			   JoinType jointype, SpecialJoinInfo *sjinfo,
 			   RelOptInfo *rel, Bitmapset **estimatedclauses)
 {
diff --git a/src/include/statistics/statistics.h b/src/include/statistics/statistics.h
index 588b6738b2..ebac910d72 100644
--- a/src/include/statistics/statistics.h
+++ b/src/include/statistics/statistics.h
@@ -104,14 +104,14 @@ extern int ComputeExtStatisticsRows(Relation onerel,
 	int natts, VacAttrStats **stats);
 extern bool statext_is_kind_built(HeapTuple htup, char kind);
 extern Selectivity dependencies_clauselist_selectivity(PlannerInfo *root,
-	   List *clauses,
+	   const List *clauses,
 	   int varRelid,
 	   JoinType jointype,
 	   SpecialJoinInfo *sjinfo,
 	   RelOptInfo *rel,
 	   Bitmapset **estimatedclauses);
 

Re: Invisible PROMPT2

[Chapman Flack]

On 11/13/19 12:49 PM, David Fetter wrote:
>> Or just define %w as meaning "whitespace of the same width as
>> PROMPT1".  You couldn't use it *in* PROMPT1, then, but I see
>> no use-case for that anyway.
> 
> +1 for doing it this way.  Would it make more sense to error out if
> somebody tried to set that in PROMPT1, or ignore it, or...?

Define it as "difference between PROMPT1's width and the total width
of non-%w elements in this prompt". Then it has a defined meaning in
PROMPT1 too (which could be arbitrary if it appears only once, but
has to be zero in case it appears more than once).

Easter egg: expand it to backspaces if used in PROMPT2 among other
stuff that's already wider than PROMPT1. ;)

Regards,
-Chap

Re: [PATCH] Improve AtSubCommit_childXids

[Andres Freund]

Hi,

On this list we quote inline, and trim quoted messages to the relevant
parts...

On 2019-11-13 17:40:27 +, Ranier Vilela wrote:
> "Why is this an improvement? And what setting are we removing? You mean
> that we reset nChildXids, even if it's already 0? Hard to see how that
> matters."
> 
> The orginal function, ever set ChildXidsm, nChildXidsa and maxChildXids.
> See at lines 1594, 1595, 1596, even if it's already 0!

So? It's easier to reason about that way anyway, and it's just about
free, because the cacheline is already touched.


> The test (nChildXids > 0), possibly works, but, may confuse when do use
> memcpy function soon after, and access one pointer that below, is checked by 
> NULL.
> How hard to see this?

But they don't necessarily have to mean the same. One is about the array
being allocated, and one is about the number of actual xids in
there. The memcpy cares about the number of xids in it. The free cares
about whether memory is allocated.

Greetings,

Andres Freund

Re: Invisible PROMPT2

[David Fetter]

On Wed, Nov 13, 2019 at 09:47:01AM -0500, Tom Lane wrote:
> ilm...@ilmari.org (Dagfinn Ilmari =?utf-8?Q?Manns=C3=A5ker?=) writes:
> > Thomas Munro  writes:
> >> From the advanced bikeshedding department: I'd like my psql
> >> transcripts to have the usual alignment, but be easier to copy and
> >> paste later without having weird prompt stuff in the middle.  How
> >> about a prompt format directive %w that means "whitespace of the same
> >> width as %/"?  Then you can make set your PROMPT2 to '%w   ' and it
> >> becomes invisible:
> 
> > That only lines up nicely if %/ is the only variable-width directive in
> > PROMPT1.
> 
> Yeah, that was my first reaction too.
> 
> > How about a circumfix directive (like the existing %[ ... %])
> > that replaces everything inside with whitespace, but keeps the width?
> 
> Or just define %w as meaning "whitespace of the same width as
> PROMPT1".  You couldn't use it *in* PROMPT1, then, but I see
> no use-case for that anyway.

+1 for doing it this way.  Would it make more sense to error out if
somebody tried to set that in PROMPT1, or ignore it, or...?

Best,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

RE: [PATCH] Improve AtSubCommit_childXids

[Ranier Vilela]

"Why is this an improvement? And what setting are we removing? You mean
that we reset nChildXids, even if it's already 0? Hard to see how that
matters."

The orginal function, ever set ChildXidsm, nChildXidsa and maxChildXids.
See at lines 1594, 1595, 1596, even if it's already 0!

The test (nChildXids > 0), possibly works, but, may confuse when do use
memcpy function soon after, and access one pointer that below, is checked by 
NULL.
How hard to see this?

Original file:
if (s->nChildXids > 0) 
memcpy(>parent->childXids[s->parent->nChildXids + 1],
   s->childXids, // s->childXids null pointer potential 
dereferencing
   s->nChildXids * sizeof(TransactionId));

s->parent->nChildXids = new_nChildXids;

/* Release child's array to avoid leakage */
if (s->childXids != NULL) // Check null pointer!
pfree(s->childXids);
/* We must reset these to avoid double-free if fail later in commit */
s->childXids = NULL; // ever set to 0
s->nChildXids = 0;  // ever set to 0
s->maxChildXids = 0; // ever set to 0

best regards,
Ranier Vilela

De: Andres Freund 
Enviado: quarta-feira, 13 de novembro de 2019 17:10
Para: Ranier Vilela
Cc: PostgreSQL Hackers
Assunto: Re: [PATCH] Improve AtSubCommit_childXids

Hi,

On 2019-11-13 16:18:46 +, Ranier Vilela wrote:
> Surely that "s->nChildXids > 0", protects s->childXids to be NULL!
> But, when we exchange the test (s->nChildXids > 0) by (s->childXids != NULL), 
> I believe we have the same protection, because, if "s->childXids" is not 
> NULL, "s->nChildXids" is > 0, naturally.
>
> That way we can improve the function and avoid calling and setting 
> unnecessarily!

Why is this an improvement? And what setting are we removing? You mean
that we reset nChildXids, even if it's already 0? Hard to see how that
matters.


> Bonus: silent compiler warning potential null pointer derenferencing.

Which compiler issues a warning here?

Greetings,

Andres Freund

Re: global / super barriers (for checksums)

[Robert Haas]

On Tue, Oct 30, 2018 at 1:17 AM Andres Freund  wrote:
> The patch definitely is in a prototype stage. At the very least it needs
> a high-level comment somewhere, and some of the lower-level code needs
> to be cleaned up.
>
> One thing I wasn't happy about is how checksum internals have to absorb
> barrier requests - that seems unavoidable, but I'd hope for something
> more global than just BufferSync().

Hi,

TL;DR: I'm not sure that we need 0001; I propose to commit 0002; and I
have some concerns about 0003 and am interested in working further on
it.

0001 changes the StartBackgroundWorker so that the SIGINT handler is
contingent on BGWORKER_SHMEM_ACCESS rather than
BGWORKER_BACKEND_DATABASE_CONNECTION. It seems to me that the goal
here should be to use procsignal_sigusr1_handler(), or something that
calls it, in any process where ProcSignalInit() is called, but a
backend that only requests BGWORKER_SHMEM_ACCESS probably won't,
because the normal way for a background worker to call
ProcSignalInit() would be to indirectly call InitPostgres() by means
of BackgroundWorkerInitializeConnection() or
BackgroundWorkerInitializeConnectionByOid(), and a worker that didn't
ask for a database connection presumably shouldn't be doing that. So
I'm not sure that I understand why we need this. (Is it legal for a
worker to call one of these functions as long as it passes InvalidOid
for the database OID, or something? Do we have examples of workers
that do that?)

On the other hand, 0002 seems like it's pretty clearly a good idea. It
makes a whole bunch of auxiliary processes use
procsignal_sigusr1_handler() and those things all get called from
AuxiliaryProcessMain(), which does ProcSignalInit(), and it seems like
clearly the right idea that processes which register to participate in
the procsignal mechanism should also register to get notified if they
receive a procsignal. I think that the reason we haven't bothered with
this up until now is because I think that it's presently impossible
for any of the kind of procsignals that we have to get sent to any of
those processes. But, global barriers would require us to do so, so it
seems like it's time to tighten that up, and it doesn't really cost
anything. So I propose to commit this part soon, unless somebody
objects.

Regarding 0003:

- The proc->pid == 0 check in EmitGlobalBarrier() doesn't really do
what it appears to do, because regular backends don't clear proc->pid
when they exit; only auxiliary processes do. (We could, and perhaps
should, change that.)

- It seems  to me that it would be generally better to insert
CHECK_FOR_INTERRUPTS() in places where the patch just inserts an
ad-hoc if (GlobalBarrierInterruptPending)
ProcessGlobalBarrierIntterupt(). There might be someplace where we
have to do it the latter way because we unavoidably hold an LWLock or
something, but I think we should avoid that whenever possible. If it's
a good place to check for one kind of interrupt, it's probably a good
place to check for all of them.

- I don't think BufferSync() is doing this in the right place. Unless
I am confused, it's doing it inside a loop that just allocates stuff
and copies data around, which probably does not need this kind of
decoration. It wouldn't hurt to check here, and it might be a good
idea for safety, but I think the place that really needs this
treatment is the following loop where we are actually doing I/O and
sleeping. Possibly we should even be doctoring CheckpointWriteDelay to
use a latch-wait loop that can CHECK_FOR_INTERRUPTS() before
re-sleeping, although for 100ms (curiously described as a Big Sleep)
it might be overkill.

- I think it would be nice to make this system more extensible. IIUC,
the idea is that ProcessGlobalBarrierIntterupt() will call bespoke
code for each GLOBBAR_* flag that set, and that code will then update
process-local state from global state before returning. But, instead
of hard-coding constants, how about just having a list of callbacks,
and we call them all here, and each one is responsible for figuring
out whether anything's been changed that it cares about, and if so
updating the appropriate local state? Then GlobalBarrierKind goes away
altogether. Granted, this could be a bit expensive if we were using
global barriers for lots of different things and at least some of
those things occurred frequently, but I don't think we're very near
the point where we need that kind of optimization. As long as the
callbacks have tests that exit quickly if there's nothing to do, I
think it should be fine. (As an intermediate position, we could
consider keeping barrierFlags but assign the bits dynamically; but
unless and until we get more users of the facility, I think it might
be simplest to just forget about barrierFlags altogether. Then even
extensions can use this, if they want.)

- The patch needs some general tidying up, like comments and naming
consistency and stuff like that.

Andres, Magnus, if neither of you are planning to work on 

Re: [PATCH] Improve AtSubCommit_childXids

[Andres Freund]

Hi,

On 2019-11-13 16:18:46 +, Ranier Vilela wrote:
> Surely that "s->nChildXids > 0", protects s->childXids to be NULL!
> But, when we exchange the test (s->nChildXids > 0) by (s->childXids != NULL), 
> I believe we have the same protection, because, if "s->childXids" is not 
> NULL, "s->nChildXids" is > 0, naturally.
> 
> That way we can improve the function and avoid calling and setting 
> unnecessarily!

Why is this an improvement? And what setting are we removing? You mean
that we reset nChildXids, even if it's already 0? Hard to see how that
matters.


> Bonus: silent compiler warning potential null pointer derenferencing.

Which compiler issues a warning here?

Greetings,

Andres Freund

Re: make pg_attribute_noreturn() work for msvc?

[Alvaro Herrera]

On 2019-Nov-12, Andres Freund wrote:

> > Anyway, I still like the idea of merging the void keyword in with
> > that.
> 
> Hm. Any other opinions?

Although it feels very strange to me at first glance, one only has to
learn the trick once.  My initial inclination was not to do it, but I'm
kinda +0.1 after thinking some more about it.

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

[PATCH] Improve AtSubCommit_childXids

[Ranier Vilela]

Hi,
Surely that "s->nChildXids > 0", protects s->childXids to be NULL!
But, when we exchange the test (s->nChildXids > 0) by (s->childXids != NULL), I 
believe we have the same protection, because, if "s->childXids" is not NULL, 
"s->nChildXids" is > 0, naturally.

That way we can improve the function and avoid calling and setting 
unnecessarily!

Bonus: silent compiler warning potential null pointer derenferencing.

Best regards,
Ranier Vilela

--- \dll\postgresql-12.0\a\backend\access\transam\xact.cMon Sep 30 
17:06:55 2019
+++ xact.c  Wed Nov 13 13:03:28 2019
@@ -1580,20 +1580,20 @@
 */
s->parent->childXids[s->parent->nChildXids] = 
XidFromFullTransactionId(s->fullTransactionId);
 
-   if (s->nChildXids > 0)
+   if (s->childXids != NULL) {
memcpy(>parent->childXids[s->parent->nChildXids + 1],
   s->childXids,
   s->nChildXids * sizeof(TransactionId));
+   /* Release child's array to avoid leakage */
+pfree(s->childXids);
 
+   /* We must reset these to avoid double-free if fail later in commit 
*/
+   s->childXids = NULL;
+   s->nChildXids = 0;
+   s->maxChildXids = 0;
+}
+   Assert(s->nChildXids == 0 && s->maxChildXids == 0);
s->parent->nChildXids = new_nChildXids;
-
-   /* Release child's array to avoid leakage */
-   if (s->childXids != NULL)
-   pfree(s->childXids);
-   /* We must reset these to avoid double-free if fail later in commit */
-   s->childXids = NULL;
-   s->nChildXids = 0;
-   s->maxChildXids = 0;
 }
 
 /* 


xact.c.patch
Description: xact.c.patch

Re: [Doc] pg_restore documentation didn't explain how to use connection string

[Lætitia Avrot]

Hi all,

So after some thoughts I did the minimal patch (for now).
I corrected documentation for the following tools so that now, using
connection string for Postgres client applications is documented in
Postgres:
- clusterdb
- pgbench
- pg_dump
- pg_restore
- reindexdb
- vacuumdb

You'll find it enclosed.

I just think it's too bad you can't use the same syntax with every Postgres
client using connection string. If somebody else feel the same way about
it, please jump into this thread so we can think together how to achieve
this.

Have a nice day,

Lætitia

Le ven. 17 mai 2019 à 09:16, Lætitia Avrot  a
écrit :

> Hi all,
>
> It seems my approach was quite candid because, of all postgres client
> applications, some document usage of connection string whereas other don't.
> Then, several ways of using connection strings are involved.
>
> Here is a little digest:
>
> | Postgres Client Application | Connection string syntax
>  | Documented ? |
>
> |-||--|
> | clusterdb   | clusterdb -d  or
> clusterdb   | No   |
> | createdb| createdb --maintenance-db
>   | No   |
> | createuser  | Couldn't find if possible
> | No   |
> | dropdb  | dropdb --maintenance-db
> | No   |
> | dropuser| Couldn't find if possible
> | No   |
> | pg_basebackup   | pg_basebackup -d 
>  | Yes  |
> | pgbench | Couldn't find if possible
> | No   |
> | pg_dump | pg_dump -d 
>  | Yes  |
> | pg_dumpall  | pg_dumpall -d 
> | Yes  |
> | pg_isready  | pg_isready -d 
> | Yes  |
> | pg_receivewal   | pg_receivewal -d 
>  | Yes  |
> | pg_recvlogical  | pg_recvlogical -d 
> | Yes  |
> | pg_restore  | pg_restore -d 
> | No   |
> | psql| psql  or psql -d
> | Yes  |
> | reindexdb   | reindexdb -d  or
> reindexdb --maintenance-db  | No   |
> | vacuumdb| vacuumdb -d  or
> vacuumdb --maintenance-db| No   |
>
> And here are some statistics about connection string usage:
>
> |  | Number of tool using that syntax |
> |--|--|
> | No switch| 2|
> | -d   | 11   |
> | --maintenance-db | 4|
>
> - Both tools that allow connection strings without strings also allow the
> -d switch.
> - From the 4 tools that use the --maintenance-db switch, only 2 won't
> allow the -d switch. Those don't have a -d switch now.
>
> Given that, I think it would be a good thing to generalize the -d switch
> (and maybe the --maintenance-db switch too).
>
> What do you think ?
>
> Cheers,
>
> Lætitia
>
> Le mar. 30 avr. 2019 à 19:10, Lætitia Avrot  a
> écrit :
>
>> Hi all,
>>
>> I'm a big fan a service file to connect to PostgreSQL client
>> applications. However I know just a few people use them.
>>
>> I ran into an issue today: I wanted to user pg_restore with my service
>> file and couldn't find a way to do so.
>>
>> Documentation didn't help. It was all about "basic" options like
>> providing host, port, user and database... Nothing about how to connect
>> using a connection string.
>>
>> I tried `pg_restore service=my_db  `, but it
>> didn't work. `pg_restore` complaining about too many arguments.
>>
>> I had to ask people or IRC to find out that the `-d` switch accepted
>> connection strings.
>>
>> It's really disturbing because :
>> - It's undocumented
>> - It doesn't work the way it works with the other PostgreSQL client
>> applications (For example, `pg_dump` will accept `pg_dump service=my_db
>> `)
>>
>> ***I write a quick patch to document that feature***, but maybe we could
>> go further. I suggest :
>>
>> - Creating a "Connection Options" section before the other options (as
>> the synopsis is pg_restore [*connection-option*...] [*option*...] [
>> *filename*])
>> - Put all connection parameters here (including the -d switch witch is
>> somehow in the middle of the other options
>> - Change other PostgreSQL client application 

Re: Using multiple extended statistics for estimates

[Tomas Vondra]

Hi,

here's an updated patch, with some minor tweaks based on the review and
added tests (I ended up reworking those a bit, to make them more like
the existing ones).

There's also a new piece, dealing with functional dependencies. Until
now we did the same thing as for MCV lists - we picketd the "best"
extended statistics (with functional dependencies built) and just used
that. At first I thought we might simply do the same loop as for MCV
lists, but that does not really make sense because we might end up
applying "weaker" dependency first.

Say for example we have table with columns (a,b,c,d,e) and functional
dependencies on (a,b,c,d) and (c,d,e) where all the dependencies on
(a,b,c,d) are weaker than (c,d => e). In a query with clauses on all
attributes this is guaranteed to apply all dependencies from the first
statistic first, which si clearly wrong.

So what this does instead is simply merging all the dependencies from
all the relevant stats, and treating them as a single collection.


regards

--
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>From 0e09c4749f3712da1983374a2838e4b7e14b7c62 Mon Sep 17 00:00:00 2001
From: Tomas Vondra 
Date: Tue, 12 Nov 2019 22:57:06 +0100
Subject: [PATCH 01/10] Apply multiple multivariate MCV lists when possible

Until now we've only used a single multivariate MCV list per relation,
covering the largest number of clauses. So for example given a query

SELECT * FROM t WHERE a = 1 AND b =1 AND c = 1 AND d = 1

and extended statistics on (a,b) and (c,d), we'd only pick and use only
one of them. This commit relaxes this by repeating the process, using
the best statistics (matching the largest number of remaining clauses)
in each step.

This greedy algorithm is very simple, but may not be optimal. There may
be a different choice of stats leaving fewer clauses unestimated and/or
giving better estimates for some other reason.

This can however happen only when there are overlapping statistics, and
selecting one makes it impossible to use the other. E.g. with statistics
on (a,b), (c,d), (b,c,d), we may pick either (a,b) and (c,d) or (b,c,d).
But it's not clear which option is better, though, as each one ignores
information about possible correlation between different columns.

We however assume cases like this are rare, and the easiest solution is
to define statistics covering the whole group of correlated columns for
a given query. In the future we might support overlapping stats, using
some of the clauses as conditions (in conditional probability sense).

Author: Tomas Vondra
Reviewed-by: Mark Dilger
Discussion: https://postgr.es/m/20191028152048.jc6pqv5hb7j77ocp@development
---
 src/backend/statistics/extended_stats.c | 166 ++--
 src/test/regress/expected/stats_ext.out |  58 +
 src/test/regress/sql/stats_ext.sql  |  36 +
 3 files changed, 195 insertions(+), 65 deletions(-)

diff --git a/src/backend/statistics/extended_stats.c 
b/src/backend/statistics/extended_stats.c
index 207ee3160e..b55799b8b1 100644
--- a/src/backend/statistics/extended_stats.c
+++ b/src/backend/statistics/extended_stats.c
@@ -1123,6 +1123,33 @@ statext_is_compatible_clause(PlannerInfo *root, Node 
*clause, Index relid,
return true;
 }
 
+/*
+ * statext_mcv_clause_attnums
+ * Recalculate attnums from compatible but not-yet-estimated 
clauses.
+ */
+static Bitmapset *
+statext_mcv_remaining_attnums(int nclauses, Bitmapset **estimatedclauses,
+ Bitmapset 
**list_attnums)
+{
+   int listidx;
+   Bitmapset  *attnums = NULL;
+
+   for (listidx = 0; listidx < nclauses; listidx++)
+   {
+   /*
+* Skip clauses that have no precalculated attnums, which means 
it is
+* either incompatible or was already used by some other 
statistic.
+*/
+   if (!list_attnums[listidx])
+   continue;
+
+   if (!bms_is_member(listidx, *estimatedclauses))
+   attnums = bms_add_members(attnums, 
list_attnums[listidx]);
+   }
+
+   return attnums;
+}
+
 /*
  * statext_mcv_clauselist_selectivity
  * Estimate clauses using the best multi-column statistics.
@@ -1173,11 +1200,6 @@ statext_is_compatible_clause(PlannerInfo *root, Node 
*clause, Index relid,
  * 'estimatedclauses' is an input/output parameter.  We set bits for the
  * 0-based 'clauses' indexes we estimate for and also skip clause items that
  * already have a bit set.
- *
- * XXX If we were to use multiple statistics, this is where it would happen.
- * We would simply repeat this on a loop on the "remaining" clauses, possibly
- * using the already estimated clauses as conditions (and combining the values
- * using conditional probability formula).
  */
 static Selectivity
 

Re: Invisible PROMPT2

[Tom Lane]

ilm...@ilmari.org (Dagfinn Ilmari =?utf-8?Q?Manns=C3=A5ker?=) writes:
> Thomas Munro  writes:
>> From the advanced bikeshedding department: I'd like my psql
>> transcripts to have the usual alignment, but be easier to copy and
>> paste later without having weird prompt stuff in the middle.  How
>> about a prompt format directive %w that means "whitespace of the same
>> width as %/"?  Then you can make set your PROMPT2 to '%w   ' and it
>> becomes invisible:

> That only lines up nicely if %/ is the only variable-width directive in
> PROMPT1.

Yeah, that was my first reaction too.

> How about a circumfix directive (like the existing %[ ... %])
> that replaces everything inside with whitespace, but keeps the width?

Or just define %w as meaning "whitespace of the same width as
PROMPT1".  You couldn't use it *in* PROMPT1, then, but I see
no use-case for that anyway.

regards, tom lane

Re: dropdb --force

[Pavel Stehule]

st 13. 11. 2019 v 7:13 odesílatel Pavel Stehule 
napsal:

>
>
> st 13. 11. 2019 v 7:12 odesílatel Amit Kapila 
> napsal:
>
>> On Tue, Nov 12, 2019 at 11:17 AM Amit Kapila 
>> wrote:
>> >
>> > I am planning to commit this patch tomorrow unless I see more comments
>> > or interest from someone else to review this.
>> >
>>
>> Pushed.  Pavel, feel free to submit dropdb utility-related patch if you
>> want.
>>
>
> I hope I send this patch today. It's simply job.
>

here it is. It's based on Filip Rembialkowski's patch if I remember
correctly

Pavel



> Pavel
>
>
>> --
>> With Regards,
>> Amit Kapila.
>> EnterpriseDB: http://www.enterprisedb.com
>>
>
diff --git a/doc/src/sgml/ref/dropdb.sgml b/doc/src/sgml/ref/dropdb.sgml
index 3fbdb33716..5d10ad1c92 100644
--- a/doc/src/sgml/ref/dropdb.sgml
+++ b/doc/src/sgml/ref/dropdb.sgml
@@ -86,6 +86,20 @@ PostgreSQL documentation
   
  
 
+ 
+  -f
+  --force
+  
+   
+Force termination of connected backends before removing the database.
+   
+   
+This will add the FORCE option to the DROP
+DATABASE command sent to the server.
+   
+  
+ 
+
  
-V
--version
diff --git a/src/bin/scripts/dropdb.c b/src/bin/scripts/dropdb.c
index dacd8e5f1d..3a6aac8ac3 100644
--- a/src/bin/scripts/dropdb.c
+++ b/src/bin/scripts/dropdb.c
@@ -34,6 +34,7 @@ main(int argc, char *argv[])
 		{"interactive", no_argument, NULL, 'i'},
 		{"if-exists", no_argument, _exists, 1},
 		{"maintenance-db", required_argument, NULL, 2},
+		{"force", no_argument, NULL, 'f'},
 		{NULL, 0, NULL, 0}
 	};
 
@@ -49,6 +50,7 @@ main(int argc, char *argv[])
 	enum trivalue prompt_password = TRI_DEFAULT;
 	bool		echo = false;
 	bool		interactive = false;
+	bool		force = false;
 
 	PQExpBufferData sql;
 
@@ -61,7 +63,7 @@ main(int argc, char *argv[])
 
 	handle_help_version_opts(argc, argv, "dropdb", help);
 
-	while ((c = getopt_long(argc, argv, "h:p:U:wWei", long_options, )) != -1)
+	while ((c = getopt_long(argc, argv, "h:p:U:wWeif", long_options, )) != -1)
 	{
 		switch (c)
 		{
@@ -86,6 +88,9 @@ main(int argc, char *argv[])
 			case 'i':
 interactive = true;
 break;
+			case 'f':
+force = true;
+break;
 			case 0:
 /* this covers the long options */
 break;
@@ -123,9 +128,6 @@ main(int argc, char *argv[])
 
 	initPQExpBuffer();
 
-	appendPQExpBuffer(, "DROP DATABASE %s%s;",
-	  (if_exists ? "IF EXISTS " : ""), fmtId(dbname));
-
 	/* Avoid trying to drop postgres db while we are connected to it. */
 	if (maintenance_db == NULL && strcmp(dbname, "postgres") == 0)
 		maintenance_db = "template1";
@@ -134,6 +136,12 @@ main(int argc, char *argv[])
 	  host, port, username, prompt_password,
 	  progname, echo);
 
+	/* now, only FORCE option can be used, so usage is very simple */
+	appendPQExpBuffer(, "DROP DATABASE %s%s%s;",
+	  (if_exists ? "IF EXISTS " : ""),
+	  fmtId(dbname),
+	  force ? " WITH (FORCE)" : "");
+
 	if (echo)
 		printf("%s\n", sql.data);
 	result = PQexec(conn, sql.data);
@@ -159,6 +167,7 @@ help(const char *progname)
 	printf(_("\nOptions:\n"));
 	printf(_("  -e, --echoshow the commands being sent to the server\n"));
 	printf(_("  -i, --interactive prompt before deleting anything\n"));
+	printf(_("  -f, --force   force termination of connected backends\n"));
 	printf(_("  -V, --version output version information, then exit\n"));
 	printf(_("  --if-exists   don't report error if database doesn't exist\n"));
 	printf(_("  -?, --helpshow this help, then exit\n"));
diff --git a/src/bin/scripts/t/050_dropdb.pl b/src/bin/scripts/t/050_dropdb.pl
index 25aa54a4ae..c51babe093 100644
--- a/src/bin/scripts/t/050_dropdb.pl
+++ b/src/bin/scripts/t/050_dropdb.pl
@@ -3,7 +3,7 @@ use warnings;
 
 use PostgresNode;
 use TestLib;
-use Test::More tests => 11;
+use Test::More tests => 13;
 
 program_help_ok('dropdb');
 program_version_ok('dropdb');
@@ -19,5 +19,11 @@ $node->issues_sql_like(
 	qr/statement: DROP DATABASE foobar1/,
 	'SQL DROP DATABASE run');
 
+$node->safe_psql('postgres', 'CREATE DATABASE foobar2');
+$node->issues_sql_like(
+	[ 'dropdb', '--force', 'foobar2' ],
+	qr/statement: DROP DATABASE foobar2 WITH \(FORCE\);/,
+	'SQL DROP DATABASE (FORCE) run');
+
 $node->command_fails([ 'dropdb', 'nonexistent' ],
 	'fails with nonexistent database');

Re: segfault in geqo on experimental gcc animal

[Fabien COELHO]

so it sure looks like a gcc upgrade caused the failure. But it's not
clear wheter it's a compiler bug, or some undefined behaviour that
triggers the bug.

Fabien, any chance to either bisect or get a bit more information on 
the backtrace?


There is a promising "keep_error_builds" option in buildfarm settings, 
but it does not seem to be used anywhere in the scripts. Well, I can 
probably relaunch by hand.


However, given the experimental nature of the setup, I think that the 
most probable cause is a newly introduced gcc bug, so I'd suggest to 
wait to check whether the issue persist before spending time on that, 
and if it persists to investigate further to either report a bug to gcc 
or pg, depending.


Also, I'll recompile gcc before the next weekly builds.


I did some manual testing.

All versions are tested failed miserably (I tested master, 12, 11, 10, 
9.6…). High probability that it is a newly introduced gcc bug, however pg 
is not a nice self contain tested case to submit to gcc for debugging:-(


I suggest to ignore for the time being, and if the problem persist I'll 
try to investigate to detect which gcc commit caused the regression.


--
Fabien.

Re: checking my understanding of TupleDesc

[Tom Lane]

Andres Freund  writes:
> On 2019-11-12 18:20:56 -0500, Tom Lane wrote:
>> Ah, right.  Probably because we need to insist on every column of an
>> execution-time tupdesc having a valid atttypid ... although I wonder,
>> is that really necessary?

> Yea, the stated reasoning is ExecTypeFromTL():
> [ ExecTypeFromTL needs to see subexpressions with valid data types ]

> I wonder if we could get away with making build_physical_tlist()
> returning a TargetEntry for a Const instead of a Var for the dropped
> columns? That'd contain enough information for tuple deforming to work
> on higher query levels?  Or perhaps we ought to invent a DroppedVar
> node, that includes the type information? That'd make it trivial to
> error out when such an expression is actually evaluated, and allow to
> detect such columns.  We already put Const nodes in some places like
> that IIRC...

Yeah, a DroppedVar thing might not be a bad idea, it could substitute
for the dummy null constants we currently use.  Note that an interesting
property of such a node is that it doesn't actually *have* a type.
A dropped column might be of a type that's been dropped too (and,
if memory serves, we reset the column's atttypid to zero anyway).
What we'd have to do is excavate atttyplen and attalign from the
pg_attribute entry and store those in the DroppedVar node.  Then,
anything reconstructing a tupdesc would have to use those fields
and avoid a pg_type lookup.

I'm not sure whether the execution-time behavior of such a node
ought to be "throw error" or just "return NULL".  The precedent
of the dummy constants suggests the latter.  What would error out
is anything that wants to extract an actual type OID from the
expression.

regards, tom lane

Re: [PATCH] use separate PartitionedRelOptions structure to store partitioned table options

[Nikolay Shaplov]

В письме от среда, 13 ноября 2019 г. 16:30:29 MSK пользователь Michael Paquier 
написал:
> On Tue, Nov 12, 2019 at 01:50:03PM +0900, Michael Paquier wrote:
> > We have been through great length to have build_reloptions, so
> > wouldn't it be better to also have this code path do so?  Sure you
> > need to pass NULL for the parsing table..  But there is a point to
> > reduce the code paths using directly parseRelOptions() and the
> > follow-up, expected calls to allocate and to fill in the set of
> > reloptions.
> 
> So I have been looking at this one, and finished with the attached.
> It looks much better to use build_reloptions() IMO, taking advantage
> of the same sanity checks present for the other relkinds.
Thanks!

I did not read that thread yet, when I sent v3 patch.
build_reloptions is a good stuff and we should use it for sure.

I've looked at yours v4 version of the patch, it is exactly what we need here. 
Can we commit it as it is?


-- 
Software Developer: https://www.upwork.com/freelancers/~014a87e140ff02c0da
Body-oriented Therapist: https://vk.com/nataraj_rebalancing  (Russian)

Re: [PATCH] Do not use StdRdOptions in Access Methods

[Nikolay Shaplov]

В письме от среда, 13 ноября 2019 г. 16:05:20 MSK пользователь Michael Paquier 
написал:

Guys! Sorry for being away for so long. I did not have much opportunities to 
pay my attention to postgres recently.

Thank you for introducing build_reloptions function. It is approximately the 
direction I wanted to move afterwards myself. 

But nevertheless, I am steady on my way, and I want to get rid of StdRdOptions 
before doing anything else myself. This structure is long outdated and is not 
suitable for access method's options at all.

I've changed the patch to use build_reloptions function and again propose it 
to the commitfest.



-- 
Software Developer: https://www.upwork.com/freelancers/~014a87e140ff02c0da
Body-oriented Therapist: https://vk.com/nataraj_rebalancing  (Russian)diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index d8790ad..f40d68c 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -23,7 +23,7 @@
 #include "access/htup_details.h"
 #include "access/nbtree.h"
 #include "access/reloptions.h"
-#include "access/spgist.h"
+#include "access/spgist_private.h"
 #include "catalog/pg_type.h"
 #include "commands/defrem.h"
 #include "commands/tablespace.h"
@@ -1510,8 +1510,6 @@ default_reloptions(Datum reloptions, bool validate, relopt_kind kind)
 		offsetof(StdRdOptions, user_catalog_table)},
 		{"parallel_workers", RELOPT_TYPE_INT,
 		offsetof(StdRdOptions, parallel_workers)},
-		{"vacuum_cleanup_index_scale_factor", RELOPT_TYPE_REAL,
-		offsetof(StdRdOptions, vacuum_cleanup_index_scale_factor)},
 		{"vacuum_index_cleanup", RELOPT_TYPE_BOOL,
 		offsetof(StdRdOptions, vacuum_index_cleanup)},
 		{"vacuum_truncate", RELOPT_TYPE_BOOL,
diff --git a/src/backend/access/hash/hashpage.c b/src/backend/access/hash/hashpage.c
index 63697bf..0cc4cb6 100644
--- a/src/backend/access/hash/hashpage.c
+++ b/src/backend/access/hash/hashpage.c
@@ -358,7 +358,7 @@ _hash_init(Relation rel, double num_tuples, ForkNumber forkNum)
 	data_width = sizeof(uint32);
 	item_width = MAXALIGN(sizeof(IndexTupleData)) + MAXALIGN(data_width) +
 		sizeof(ItemIdData);		/* include the line pointer */
-	ffactor = RelationGetTargetPageUsage(rel, HASH_DEFAULT_FILLFACTOR) / item_width;
+	ffactor = (BLCKSZ * HashGetFillFactor(rel) / 100) / item_width;
 	/* keep to a sane range */
 	if (ffactor < 10)
 		ffactor = 10;
diff --git a/src/backend/access/hash/hashutil.c b/src/backend/access/hash/hashutil.c
index c5005f4..669dccc 100644
--- a/src/backend/access/hash/hashutil.c
+++ b/src/backend/access/hash/hashutil.c
@@ -289,7 +289,14 @@ _hash_checkpage(Relation rel, Buffer buf, int flags)
 bytea *
 hashoptions(Datum reloptions, bool validate)
 {
-	return default_reloptions(reloptions, validate, RELOPT_KIND_HASH);
+	static const relopt_parse_elt tab[] = {
+		{"fillfactor", RELOPT_TYPE_INT, offsetof(HashOptions, fillfactor)},
+	};
+
+	return (bytea *) build_reloptions(reloptions, validate,
+	  RELOPT_KIND_HASH,
+	  sizeof(HashOptions),
+	  tab, lengthof(tab));
 }
 
 /*
diff --git a/src/backend/access/nbtree/nbtree.c b/src/backend/access/nbtree/nbtree.c
index 4cfd528..8352882 100644
--- a/src/backend/access/nbtree/nbtree.c
+++ b/src/backend/access/nbtree/nbtree.c
@@ -816,7 +816,7 @@ _bt_vacuum_needs_cleanup(IndexVacuumInfo *info)
 	}
 	else
 	{
-		StdRdOptions *relopts;
+		BTOptions *relopts;
 		float8		cleanup_scale_factor;
 		float8		prev_num_heap_tuples;
 
@@ -827,7 +827,7 @@ _bt_vacuum_needs_cleanup(IndexVacuumInfo *info)
 		 * tuples exceeds vacuum_cleanup_index_scale_factor fraction of
 		 * original tuples count.
 		 */
-		relopts = (StdRdOptions *) info->index->rd_options;
+		relopts = (BTOptions *) info->index->rd_options;
 		cleanup_scale_factor = (relopts &&
 relopts->vacuum_cleanup_index_scale_factor >= 0)
 			? relopts->vacuum_cleanup_index_scale_factor
diff --git a/src/backend/access/nbtree/nbtsort.c b/src/backend/access/nbtree/nbtsort.c
index c11a3fb..9906c80 100644
--- a/src/backend/access/nbtree/nbtsort.c
+++ b/src/backend/access/nbtree/nbtsort.c
@@ -716,8 +716,9 @@ _bt_pagestate(BTWriteState *wstate, uint32 level)
 	if (level > 0)
 		state->btps_full = (BLCKSZ * (100 - BTREE_NONLEAF_FILLFACTOR) / 100);
 	else
-		state->btps_full = RelationGetTargetPageFreeSpace(wstate->index,
-		  BTREE_DEFAULT_FILLFACTOR);
+		state->btps_full = (BLCKSZ * (100 - BTGetFillFactor(wstate->index))
+			 / 100);
+
 	/* no parent level, yet */
 	state->btps_next = NULL;
 
diff --git a/src/backend/access/nbtree/nbtsplitloc.c b/src/backend/access/nbtree/nbtsplitloc.c
index a04d4e2..29167f1 100644
--- a/src/backend/access/nbtree/nbtsplitloc.c
+++ b/src/backend/access/nbtree/nbtsplitloc.c
@@ -167,7 +167,7 @@ _bt_findsplitloc(Relation rel,
 
 	/* Count up total space in data items before actually scanning 'em */
 	olddataitemstotal = rightspace - (int) PageGetExactFreeSpace(page);
-	leaffillfactor = RelationGetFillFactor(rel, 

Re: ssl passphrase callback

[Simon Riggs]

On Wed, 13 Nov 2019 at 13:08, Bruce Momjian  wrote:

> On Tue, Nov 12, 2019 at 09:51:33PM -0500, Bruce Momjian wrote:
> > On Sun, Nov 10, 2019 at 01:01:17PM -0600, Magnus Hagander wrote:
> > > On Wed, Nov 6, 2019 at 7:24 PM Bruce Momjian  wrote:
>
> > > One of the main reasons there being to be easily able to transfer more
> state
> > > and give results other than just an exit code, no need to deal with
> parameter
> > > escaping etc. Which probably wouldn't matter as much to an SSL
> passphrase
> > > command, but still.
> >
> > I get the callback-is-easier issue with shared objects, but are we
> > expecting to pass in more information here than we do for
> > archive_command?  I would think not.  What I am saying is that if we
> > don't think passing things in works, we should fix all these external
> > commands, or something.   I don't see why ssl_passphrase_command is
> > different, except that it is new.



> Or is it related to _securely_passing something?
>

Yes


> > Also, why was this patch posted without any discussion of these issues?
> > Shouldn't we ideally discuss the API first?
>
> I wonder if every GUC that takes an OS command should allow a shared
> object to be specified --- maybe control that if the command string
> starts with a # or something.
>

Very good idea

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

PostgreSQL Solutions for the Enterprise

Re: ssl passphrase callback

[Bruce Momjian]

On Tue, Nov 12, 2019 at 09:51:33PM -0500, Bruce Momjian wrote:
> On Sun, Nov 10, 2019 at 01:01:17PM -0600, Magnus Hagander wrote:
> > On Wed, Nov 6, 2019 at 7:24 PM Bruce Momjian  wrote:
> >   We had this
> > discussion in relation to archive_command years ago, and decided on a
> > shell command as the best API.
> >
> > I don't recall that from back then, but that was a long time ago.
> > 
> > But it's interesting that you mention it, given the number of people I have
> > been discussing that with recently, under the topic of changing it from a 
> > shell
> > command into a shared library API (with there being a shell command as one
> > possible implementation of course). 
> > 
> > One of the main reasons there being to be easily able to transfer more state
> > and give results other than just an exit code, no need to deal with 
> > parameter
> > escaping etc. Which probably wouldn't matter as much to an SSL passphrase
> > command, but still.
> 
> I get the callback-is-easier issue with shared objects, but are we
> expecting to pass in more information here than we do for
> archive_command?  I would think not.  What I am saying is that if we
> don't think passing things in works, we should fix all these external
> commands, or something.   I don't see why ssl_passphrase_command is
> different, except that it is new.  Or is it related to _securely_
> passing something?
> 
> Also, why was this patch posted without any discussion of these issues?
> Shouldn't we ideally discuss the API first?

I wonder if every GUC that takes an OS command should allow a shared
object to be specified --- maybe control that if the command string
starts with a # or something.

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

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

Re: Remove configure --disable-float4-byval and --disable-float8-byval

[Robert Haas]

On Sat, Nov 2, 2019 at 8:00 PM Andres Freund  wrote:
> I think we really ought to remove the difference behind macros. That is,
> for types that could be either, provide macros that fetch function
> arguments into local memory, independent of whether the argument is a
> byval or byref type.  I.e. instead of having separate #ifdef
> USE_FLOAT8_BYVALs for DatumGetFloat8(), DatumGetInt64(), ... we should
> provide that logic in one centralized set of macros.
>
> The fact that USE_FLOAT8_BYVAL has to creep into various functions imo
> is the reasons why people are unhappy about it.

I think I'm *more* unhappy about the fact that it affects a bunch of
things that are not about whether float8 is passed byval. I mean, you
mention DatumGetInt64() above, but why in the world does a setting
with "float8" in the name affect how we pass int64? The thing is like
kudzu, getting into all sorts of things that it has no business
affecting - at least if you judge by the name - and for no really
clear reason.

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

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

[Amit Kapila]

On Thu, Oct 3, 2019 at 1:18 PM Dilip Kumar  wrote:
>

As mentioned by me a few days back that the first patch in this series
is ready to go [1] (I am hoping Tomas will pick it up), so I have
started the review of other patches

Review/Questions on 0002-Immediately-WAL-log-assignments.patch
-
1. This patch adds the top_xid in WAL whenever the first time WAL for
a subtransaction XID is written to correctly decode the changes of
in-progress transaction.  This patch also removes logging and applying
WAL for XLOG_XACT_ASSIGNMENT which might have some effect.  As replay
of that, it prunes KnownAssignedXids to prevent overflow of that
array.  See comments in procarray.c (KnownAssignedTransactionIds
sub-module).  Can you please explain how after removing the WAL for
XLOG_XACT_ASSIGNMENT, we will handle that or I am missing something
and there is no impact of same?

2.
+#define XLOG_INCLUDE_INVALS 0x08 /* include invalidations */

This doesn't seem to be used in this patch.

[1] - 
https://www.postgresql.org/message-id/CAA4eK1JM0%3DRwODZQrn8DTQ3dbcb9xwKDdHCmVOryAk_xoKf9Nw%40mail.gmail.com

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

Re: MarkBufferDirtyHint() and LSN update

[Michael Paquier]

On Mon, Nov 11, 2019 at 10:03:14AM +0100, Antonin Houska wrote:
> This looks good to me.

Actually, no, this is not good.  I have been studying more the patch,
and after stressing more this code path with a cluster having
checksums enabled and shared_buffers at 1MB, I have been able to make
a couple of page's LSNs go backwards with pgbench -s 100.  The cause
was simply that the page got flushed with a newer LSN than what was
returned by XLogSaveBufferForHint() before taking the buffer header
lock, so updating only the LSN for a non-dirty page was simply
guarding against that.
--
Michael


signature.asc
Description: PGP signature

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

[Julien Rouhaud]

On Wed, Nov 13, 2019 at 4:15 AM Bruce Momjian  wrote:
>
> On Mon, Nov 11, 2019 at 05:37:30PM +0900, Michael Paquier wrote:
> > On Wed, Sep 11, 2019 at 06:30:22PM +0200, Julien Rouhaud wrote:
> > > The thing is that pg_stat_statements assigns a 0 queryid in the
> > > post_parse_analyze_hook to recognize utility statements and avoid
> > > tracking instrumentation twice in case of utility statements, and then
> > > compute a queryid base on a hash of the query text.  Maybe we could
> > > instead fully reserve queryid "2" for utility statements (so forcing
> > > queryid "1" for standard queries if jumbling returns 0 *or* 2 instead
> > > of only 0), and use "2" as the identifier for utility statement
> > > instead of "0"?
> >
> > Hmm.  Not sure.  At this stage it would be nice to gather more input
> > on the matter, and FWIW, I don't like much the assumption that a query
> > ID of 0 is perhaps a utility statement, or perhaps nothing depending
> > on the state of a backend entry, or even perhaps something else
> > depending how on how modules make use and define such query IDs.
>
> I thought each extension would export a function to compute the query
> id, and you would all that function with the pg_stat_activity.query
> string.

I'd really like to have the queryid function available through SQL,
but I think that this specific case wouldn't work very well for
pg_stat_statements' approach as it's working with oid.  The query
string in pg_stat_activity is the user provided one rather than a
fully-qualified version, so in order to get that query's queryid, you
need to know the exact search_path in use in that backend, and that's
not something available.

Re: Invisible PROMPT2

[Dagfinn Ilmari Mannsåker]

Thomas Munro  writes:

> Hello hackers,
>
> From the advanced bikeshedding department: I'd like my psql
> transcripts to have the usual alignment, but be easier to copy and
> paste later without having weird prompt stuff in the middle.  How
> about a prompt format directive %w that means "whitespace of the same
> width as %/"?  Then you can make set your PROMPT2 to '%w   ' and it
> becomes invisible:

That only lines up nicely if %/ is the only variable-width directive in
PROMPT1.  How about a circumfix directive (like the existing %[ ... %])
that replaces everything inside with whitespace, but keeps the width?

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

Re: [HACKERS] Block level parallel vacuum

[Masahiko Sawada]

On Wed, 13 Nov 2019 at 18:49, Dilip Kumar  wrote:
>
> On Wed, Nov 13, 2019 at 11:39 AM Masahiko Sawada
>  wrote:
> >
> > On Wed, 13 Nov 2019 at 12:43, Dilip Kumar  wrote:
> > >
> > > On Tue, Nov 12, 2019 at 5:31 PM Masahiko Sawada
> > >  wrote:
> > > >
> > > > On Tue, 12 Nov 2019 at 20:29, Dilip Kumar  wrote:
> > > > >
> > > > > On Tue, Nov 12, 2019 at 4:04 PM Masahiko Sawada
> > > > >  wrote:
> > > > > >
> > > > > > On Mon, 11 Nov 2019 at 17:57, Dilip Kumar  
> > > > > > wrote:
> > > > > > >
> > > > > > > On Tue, Oct 29, 2019 at 12:37 PM Masahiko Sawada 
> > > > > > >  wrote:
> > > > > > > > I realized that v31-0006 patch doesn't work fine so I've 
> > > > > > > > attached the
> > > > > > > > updated version patch that also incorporated some comments I 
> > > > > > > > got so
> > > > > > > > far. Sorry for the inconvenience. I'll apply your 0001 patch 
> > > > > > > > and also
> > > > > > > > test the total delay time.
> > > > > > > >
> > > > > > > While reviewing the 0002, I got one doubt related to how we are
> > > > > > > dividing the maintainance_work_mem
> > > > > > >
> > > > > > > +prepare_index_statistics(LVShared *lvshared, Relation *Irel, int 
> > > > > > > nindexes)
> > > > > > > +{
> > > > > > > + /* Compute the new maitenance_work_mem value for index 
> > > > > > > vacuuming */
> > > > > > > + lvshared->maintenance_work_mem_worker =
> > > > > > > + (nindexes_mwm > 0) ? maintenance_work_mem / nindexes_mwm :
> > > > > > > maintenance_work_mem;
> > > > > > > +}
> > > > > > > Is it fair to just consider the number of indexes which use
> > > > > > > maintenance_work_mem?  Or we need to consider the number of 
> > > > > > > worker as
> > > > > > > well.  My point is suppose there are 10 indexes which will use the
> > > > > > > maintenance_work_mem but we are launching just 2 workers then 
> > > > > > > what is
> > > > > > > the point in dividing the maintenance_work_mem by 10.
> > > > > > >
> > > > > > > IMHO the calculation should be like this
> > > > > > > lvshared->maintenance_work_mem_worker = (nindexes_mwm > 0) ?
> > > > > > > maintenance_work_mem / Min(nindexes_mwm, nworkers)  :
> > > > > > > maintenance_work_mem;
> > > > > > >
> > > > > > > Am I missing something?
> > > > > >
> > > > > > No, I think you're right. On the other hand I think that dividing it
> > > > > > by the number of indexes that will use the mantenance_work_mem makes
> > > > > > sense when parallel degree > the number of such indexes. Suppose the
> > > > > > table has 2 indexes and there are 10 workers then we should divide 
> > > > > > the
> > > > > > maintenance_work_mem by 2 rather than 10 because it's possible that 
> > > > > > at
> > > > > > most 2 indexes that uses the maintenance_work_mem are processed in
> > > > > > parallel at a time.
> > > > > >
> > > > > Right, thats the reason I suggested divide with Min(nindexes_mwm, 
> > > > > nworkers).
> > > >
> > > > Thanks! I'll fix it in the next version patch.
> > > >
> > > One more comment.
> > >
> > > +lazy_vacuum_indexes(LVRelStats *vacrelstats, Relation *Irel,
> > > + int nindexes, IndexBulkDeleteResult **stats,
> > > + LVParallelState *lps)
> > > +{
> > > + 
> > >
> > > + if (ParallelVacuumIsActive(lps))
> > > + {
> > >
> > > +
> > > + lazy_parallel_vacuum_or_cleanup_indexes(vacrelstats, Irel, nindexes,
> > > + stats, lps);
> > > +
> > > + }
> > > +
> > > + for (idx = 0; idx < nindexes; idx++)
> > > + {
> > > + /*
> > > + * Skip indexes that we have already vacuumed during parallel index
> > > + * vacuuming.
> > > + */
> > > + if (ParallelVacuumIsActive(lps) && !IndStatsIsNull(lps->lvshared, idx))
> > > + continue;
> > > +
> > > + lazy_vacuum_index(Irel[idx], [idx], vacrelstats->dead_tuples,
> > > +   vacrelstats->old_live_tuples);
> > > + }
> > > +}
> > >
> > > In this function, if ParallelVacuumIsActive, we perform the parallel
> > > vacuum for all the index for which parallel vacuum is supported and
> > > once that is over we finish vacuuming remaining indexes for which
> > > parallel vacuum is not supported.  But, my question is that inside
> > > lazy_parallel_vacuum_or_cleanup_indexes, we wait for all the workers
> > > to finish their job then only we start with the sequential vacuuming
> > > shouldn't we start that immediately as soon as the leader
> > > participation is over in the parallel vacuum?
> >
> > If we do that, while the leader process is vacuuming indexes that
> > don't not support parallel vacuum sequentially some workers might be
> > vacuuming for other indexes. Isn't it a problem?
>
> I am not sure what could be the problem.
>
>  If it's not problem,
> > I think we can tie up indexes that don't support parallel vacuum to
> > the leader and do parallel index vacuum.
>
> I am not sure whether we can do that or not.  Because if we do a
> parallel vacuum from the leader for the indexes which don't support a
> parallel option then we will unnecessarily allocate the shared memory
> for those indexes (index stats).  Moreover, I think it could also

Re: BUG #16109: Postgres planning time is high across version - 10.6 vs 10.10

[Pavel Stehule]

st 13. 11. 2019 v 11:39 odesílatel Julien Rouhaud 
napsal:

> (moved to -hackers)
>
> On Tue, Nov 12, 2019 at 9:55 PM Andres Freund  wrote:
> >
> > This last point is more oriented towards other PG developers: I wonder
> > if we ought to display buffer statistics for plan time, for EXPLAIN
> > (BUFFERS). That'd surely make it easier to discern cases where we
> > e.g. access the index and scan a lot of the index from cases where we
> > hit some CPU time issue. We should easily be able to get that data, I
> > think, we already maintain it, we'd just need to compute the diff
> > between pgBufferUsage before / after planning.
>
> That would be quite interesting to have.  I attach as a reference a
> quick POC patch to implement it:
>
> # explain (analyze, buffers) select * from pg_stat_activity;
>  QUERY PLAN
>
> 
>  Hash Left Join  (cost=2.25..3.80 rows=100 width=440) (actual
> time=0.259..0.276 rows=6 loops=1)
>Hash Cond: (s.usesysid = u.oid)
>Buffers: shared hit=5
>->  Hash Left Join  (cost=1.05..2.32 rows=100 width=376) (actual
> time=0.226..0.236 rows=6 loops=1)
>  Hash Cond: (s.datid = d.oid)
>  Buffers: shared hit=4
>  ->  Function Scan on pg_stat_get_activity s  (cost=0.00..1.00
> rows=100 width=312) (actual time=0.148..0.151 rows=6 loop
>  ->  Hash  (cost=1.02..1.02 rows=2 width=68) (actual
> time=0.034..0.034 rows=5 loops=1)
>Buckets: 1024  Batches: 1  Memory Usage: 9kB
>Buffers: shared hit=1
>->  Seq Scan on pg_database d  (cost=0.00..1.02 rows=2
> width=68) (actual time=0.016..0.018 rows=5 loops=1)
>  Buffers: shared hit=1
>->  Hash  (cost=1.09..1.09 rows=9 width=68) (actual
> time=0.015..0.015 rows=9 loops=1)
>  Buckets: 1024  Batches: 1  Memory Usage: 9kB
>  Buffers: shared hit=1
>  ->  Seq Scan on pg_authid u  (cost=0.00..1.09 rows=9
> width=68) (actual time=0.004..0.008 rows=9 loops=1)
>Buffers: shared hit=1
>  Planning Time: 1.902 ms
>Buffers: shared hit=37 read=29
>I/O Timings: read=0.506
>  Execution Time: 0.547 ms
> (21 rows)
>
> Note that there's a related discussion in the "Planning counters in
> pg_stat_statements" thread, on whether to also compute buffers from
> planning or not.
>

+1

Pavel

Re: Planning counters in pg_stat_statements (using pgss_store)

[Julien Rouhaud]

On Tue, Nov 12, 2019 at 5:41 AM imai.yoshik...@fujitsu.com
 wrote:
>
> On Sat, Nov 9, 2019 at 1:36 PM, Julien Rouhaud wrote:
> >
> > I attach v3 patches implementing those counters.
>
> Thanks for updating the patch! Now I can see min/max/mean/stddev plan time.
>
>
> > Note that to avoid duplicating some code (related to Welford's method),
> > I switched some of the counters to arrays rather than scalar variables.  It 
> > unfortunately makes
> > pg_stat_statements_internal() a little bit messy, but I hope that it's 
> > still acceptable.
>
> Yeah, I also think it's acceptable, but I think the codes like below one is 
> more
> understandable than using for loop in pg_stat_statements_internal(),
> although some codes will be duplicated.
>
> pg_stat_statements_internal():
>
> if (api_version >= PGSS_V1_8)
> {
> kind = PGSS_PLAN;
> values[i++] = Int64GetDatumFast(tmp.calls[kind]);
> values[i++] = Float8GetDatumFast(tmp.total_time[kind]);
> values[i++] = Float8GetDatumFast(tmp.min_time[kind]);
> values[i++] = Float8GetDatumFast(tmp.max_time[kind]);
> values[i++] = Float8GetDatumFast(tmp.mean_time[kind]);
> values[i++] = Float8GetDatumFast(stddev(tmp));
> }
>
> kind = PGSS_EXEC;
> values[i++] = Int64GetDatumFast(tmp.calls[kind]);
> values[i++] = Float8GetDatumFast(tmp.total_time[kind]);
> if (api_version >= PGSS_V1_3)
> {
> values[i++] = Float8GetDatumFast(tmp.min_time[kind]);
> values[i++] = Float8GetDatumFast(tmp.max_time[kind]);
> values[i++] = Float8GetDatumFast(tmp.mean_time[kind]);
> values[i++] = Float8GetDatumFast(stddev(tmp));
> }
>
>
> stddev(Counters counters)
> {
> /*
>  * Note we are calculating the population variance here, not the
>  * sample variance, as we have data for the whole population, so
>  * Bessel's correction is not used, and we don't divide by
>  * tmp.calls - 1.
>  */
> if (counters.calls[kind] > 1)
> return stddev = sqrt(counters.sum_var_time[kind] / 
> counters.calls[kind]);
>
> return 0.0;
> }

Yes, that's also a possibility (though this should also take the
"kind" as parameter).  I wanted to avoid adding a new function and
save some redundant code, but I can change it in the next version of
the patch if needed.

> > While doing this refactoring
> > I saw that previous patches were failing to accumulate the buffers used 
> > during planning, which is now fixed.
>
> Checked.
> Now buffer usage columns include buffer usage during planning and executing,
> but if we turn off track_planning, buffer usage during planning is also not
> tracked which is good for users who don't want to take into account of that.

Indeed.  Note that there's a similar discussion on adding planning
buffer counters to explain in [1].  I'm unsure if merging planning and
execution counters in the same columns is ok or not.

> What I'm concerned about is column names will not be backward-compatible.
> {total, min, max, mean, stddev}_{plan, exec}_time are the best names which
> correctly show the meaning of its value, but we can't use
> {total, min, max, mean, stddev}_time anymore and it might be harmful for
> some users.
> I don't come up with any good idea for that...

Well, perhaps keeping the old {total, min, max, mean, stddev}_time
would be ok, as those historically meant "execution".  I don't have a
strong opinion here.

[1] 
https://www.postgresql.org/message-id/20191112205506.rvadbx2dnku3p...@alap3.anarazel.de

Re: BUG #16109: Postgres planning time is high across version - 10.6 vs 10.10

[Julien Rouhaud]

(moved to -hackers)

On Tue, Nov 12, 2019 at 9:55 PM Andres Freund  wrote:
>
> This last point is more oriented towards other PG developers: I wonder
> if we ought to display buffer statistics for plan time, for EXPLAIN
> (BUFFERS). That'd surely make it easier to discern cases where we
> e.g. access the index and scan a lot of the index from cases where we
> hit some CPU time issue. We should easily be able to get that data, I
> think, we already maintain it, we'd just need to compute the diff
> between pgBufferUsage before / after planning.

That would be quite interesting to have.  I attach as a reference a
quick POC patch to implement it:

# explain (analyze, buffers) select * from pg_stat_activity;
 QUERY PLAN

 Hash Left Join  (cost=2.25..3.80 rows=100 width=440) (actual
time=0.259..0.276 rows=6 loops=1)
   Hash Cond: (s.usesysid = u.oid)
   Buffers: shared hit=5
   ->  Hash Left Join  (cost=1.05..2.32 rows=100 width=376) (actual
time=0.226..0.236 rows=6 loops=1)
 Hash Cond: (s.datid = d.oid)
 Buffers: shared hit=4
 ->  Function Scan on pg_stat_get_activity s  (cost=0.00..1.00
rows=100 width=312) (actual time=0.148..0.151 rows=6 loop
 ->  Hash  (cost=1.02..1.02 rows=2 width=68) (actual
time=0.034..0.034 rows=5 loops=1)
   Buckets: 1024  Batches: 1  Memory Usage: 9kB
   Buffers: shared hit=1
   ->  Seq Scan on pg_database d  (cost=0.00..1.02 rows=2
width=68) (actual time=0.016..0.018 rows=5 loops=1)
 Buffers: shared hit=1
   ->  Hash  (cost=1.09..1.09 rows=9 width=68) (actual
time=0.015..0.015 rows=9 loops=1)
 Buckets: 1024  Batches: 1  Memory Usage: 9kB
 Buffers: shared hit=1
 ->  Seq Scan on pg_authid u  (cost=0.00..1.09 rows=9
width=68) (actual time=0.004..0.008 rows=9 loops=1)
   Buffers: shared hit=1
 Planning Time: 1.902 ms
   Buffers: shared hit=37 read=29
   I/O Timings: read=0.506
 Execution Time: 0.547 ms
(21 rows)

Note that there's a related discussion in the "Planning counters in
pg_stat_statements" thread, on whether to also compute buffers from
planning or not.


show_planning_buffers-v1.diff
Description: Binary data

Re: [HACKERS] Block level parallel vacuum

[Amit Kapila]

On Wed, Nov 13, 2019 at 3:55 PM Masahiko Sawada
 wrote:
>
> On Wed, 13 Nov 2019 at 17:57, Amit Kapila  wrote:
> >
> > On Wed, Nov 13, 2019 at 11:39 AM Masahiko Sawada
> >  wrote:
> > >
> > > On Wed, 13 Nov 2019 at 12:43, Dilip Kumar  wrote:
> > > >
> > > >
> > > > In this function, if ParallelVacuumIsActive, we perform the parallel
> > > > vacuum for all the index for which parallel vacuum is supported and
> > > > once that is over we finish vacuuming remaining indexes for which
> > > > parallel vacuum is not supported.  But, my question is that inside
> > > > lazy_parallel_vacuum_or_cleanup_indexes, we wait for all the workers
> > > > to finish their job then only we start with the sequential vacuuming
> > > > shouldn't we start that immediately as soon as the leader
> > > > participation is over in the parallel vacuum?
> > >
> > > If we do that, while the leader process is vacuuming indexes that
> > > don't not support parallel vacuum sequentially some workers might be
> > > vacuuming for other indexes. Isn't it a problem?
> > >
> >
> > Can you please explain what problem do you see with that?
>
> I think it depends on index AM user expectation. If disabling parallel
> vacuum for an index means that index AM user doesn't just want to
> vacuum the index by parallel worker, it's not problem. But if it means
> that the user doesn't want to vacuum the index during other indexes is
>  being processed in parallel it's unexpected behaviour for the user.
>

I would expect the earlier.

> I'm probably worrying too much.
>

Yeah, we can keep the behavior with respect to your first expectation
(If disabling parallel vacuum for an index means that index AM user
doesn't just want to vacuum the index by parallel worker, it's not
problem).  It might not be difficult to change later if there is an
example of such a case.


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

[PATCH] gcc warning 'expression which evaluates to zero treated as a null pointer'

[didier]

Hi,
Trivial patch:
- remove a gcc warning (since commit 7a0574b5)
expression which evaluates to zero treated as a null pointer constant of
  type 'HeapTuple' (aka 'struct HeapTupleData *')

- always use "if (newtuple == NULL)" rather than mixing !newtuple and
newtuple == NULL

Regards
Didier
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index 84144b46b1..0467a4811b 100644
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -2533,7 +2533,7 @@ ExecBRInsertTriggers(EState *estate, ResultRelInfo *relinfo,
 	 TupleTableSlot *slot)
 {
 	TriggerDesc *trigdesc = relinfo->ri_TrigDesc;
-	HeapTuple	newtuple = false;
+	HeapTuple	newtuple = NULL;
 	bool		should_free;
 	TriggerData LocTriggerData;
 	int			i;
@@ -2563,7 +2563,7 @@ ExecBRInsertTriggers(EState *estate, ResultRelInfo *relinfo,
 			NULL, NULL, slot))
 			continue;
 
-		if (!newtuple)
+		if (newtuple == NULL)
 			newtuple = ExecFetchSlotHeapTuple(slot, true, _free);
 
 		LocTriggerData.tg_trigslot = slot;
@@ -3178,7 +3178,7 @@ ExecIRUpdateTriggers(EState *estate, ResultRelInfo *relinfo,
 {
 	TriggerDesc *trigdesc = relinfo->ri_TrigDesc;
 	TupleTableSlot *oldslot = ExecGetTriggerOldSlot(estate, relinfo);
-	HeapTuple	newtuple = false;
+	HeapTuple	newtuple = NULL;
 	bool		should_free;
 	TriggerData LocTriggerData;
 	int			i;
@@ -3207,7 +3207,7 @@ ExecIRUpdateTriggers(EState *estate, ResultRelInfo *relinfo,
 			NULL, oldslot, newslot))
 			continue;
 
-		if (!newtuple)
+		if (newtuple == NULL)
 			newtuple = ExecFetchSlotHeapTuple(newslot, true, _free);
 
 		LocTriggerData.tg_trigslot = oldslot;

Re: [HACKERS] Block level parallel vacuum

[Masahiko Sawada]

On Wed, 13 Nov 2019 at 17:57, Amit Kapila  wrote:
>
> On Wed, Nov 13, 2019 at 11:39 AM Masahiko Sawada
>  wrote:
> >
> > On Wed, 13 Nov 2019 at 12:43, Dilip Kumar  wrote:
> > >
> > >
> > > In this function, if ParallelVacuumIsActive, we perform the parallel
> > > vacuum for all the index for which parallel vacuum is supported and
> > > once that is over we finish vacuuming remaining indexes for which
> > > parallel vacuum is not supported.  But, my question is that inside
> > > lazy_parallel_vacuum_or_cleanup_indexes, we wait for all the workers
> > > to finish their job then only we start with the sequential vacuuming
> > > shouldn't we start that immediately as soon as the leader
> > > participation is over in the parallel vacuum?
> >
> > If we do that, while the leader process is vacuuming indexes that
> > don't not support parallel vacuum sequentially some workers might be
> > vacuuming for other indexes. Isn't it a problem?
> >
>
> Can you please explain what problem do you see with that?

I think it depends on index AM user expectation. If disabling parallel
vacuum for an index means that index AM user doesn't just want to
vacuum the index by parallel worker, it's not problem. But if it means
that the user doesn't want to vacuum the index during other indexes is
 being processed in parallel it's unexpected behaviour for the user.
I'm probably worrying too much.

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

Re: [HACKERS] Block level parallel vacuum

[Dilip Kumar]

On Wed, Nov 13, 2019 at 11:39 AM Masahiko Sawada
 wrote:
>
> On Wed, 13 Nov 2019 at 12:43, Dilip Kumar  wrote:
> >
> > On Tue, Nov 12, 2019 at 5:31 PM Masahiko Sawada
> >  wrote:
> > >
> > > On Tue, 12 Nov 2019 at 20:29, Dilip Kumar  wrote:
> > > >
> > > > On Tue, Nov 12, 2019 at 4:04 PM Masahiko Sawada
> > > >  wrote:
> > > > >
> > > > > On Mon, 11 Nov 2019 at 17:57, Dilip Kumar  
> > > > > wrote:
> > > > > >
> > > > > > On Tue, Oct 29, 2019 at 12:37 PM Masahiko Sawada 
> > > > > >  wrote:
> > > > > > > I realized that v31-0006 patch doesn't work fine so I've attached 
> > > > > > > the
> > > > > > > updated version patch that also incorporated some comments I got 
> > > > > > > so
> > > > > > > far. Sorry for the inconvenience. I'll apply your 0001 patch and 
> > > > > > > also
> > > > > > > test the total delay time.
> > > > > > >
> > > > > > While reviewing the 0002, I got one doubt related to how we are
> > > > > > dividing the maintainance_work_mem
> > > > > >
> > > > > > +prepare_index_statistics(LVShared *lvshared, Relation *Irel, int 
> > > > > > nindexes)
> > > > > > +{
> > > > > > + /* Compute the new maitenance_work_mem value for index vacuuming 
> > > > > > */
> > > > > > + lvshared->maintenance_work_mem_worker =
> > > > > > + (nindexes_mwm > 0) ? maintenance_work_mem / nindexes_mwm :
> > > > > > maintenance_work_mem;
> > > > > > +}
> > > > > > Is it fair to just consider the number of indexes which use
> > > > > > maintenance_work_mem?  Or we need to consider the number of worker 
> > > > > > as
> > > > > > well.  My point is suppose there are 10 indexes which will use the
> > > > > > maintenance_work_mem but we are launching just 2 workers then what 
> > > > > > is
> > > > > > the point in dividing the maintenance_work_mem by 10.
> > > > > >
> > > > > > IMHO the calculation should be like this
> > > > > > lvshared->maintenance_work_mem_worker = (nindexes_mwm > 0) ?
> > > > > > maintenance_work_mem / Min(nindexes_mwm, nworkers)  :
> > > > > > maintenance_work_mem;
> > > > > >
> > > > > > Am I missing something?
> > > > >
> > > > > No, I think you're right. On the other hand I think that dividing it
> > > > > by the number of indexes that will use the mantenance_work_mem makes
> > > > > sense when parallel degree > the number of such indexes. Suppose the
> > > > > table has 2 indexes and there are 10 workers then we should divide the
> > > > > maintenance_work_mem by 2 rather than 10 because it's possible that at
> > > > > most 2 indexes that uses the maintenance_work_mem are processed in
> > > > > parallel at a time.
> > > > >
> > > > Right, thats the reason I suggested divide with Min(nindexes_mwm, 
> > > > nworkers).
> > >
> > > Thanks! I'll fix it in the next version patch.
> > >
> > One more comment.
> >
> > +lazy_vacuum_indexes(LVRelStats *vacrelstats, Relation *Irel,
> > + int nindexes, IndexBulkDeleteResult **stats,
> > + LVParallelState *lps)
> > +{
> > + 
> >
> > + if (ParallelVacuumIsActive(lps))
> > + {
> >
> > +
> > + lazy_parallel_vacuum_or_cleanup_indexes(vacrelstats, Irel, nindexes,
> > + stats, lps);
> > +
> > + }
> > +
> > + for (idx = 0; idx < nindexes; idx++)
> > + {
> > + /*
> > + * Skip indexes that we have already vacuumed during parallel index
> > + * vacuuming.
> > + */
> > + if (ParallelVacuumIsActive(lps) && !IndStatsIsNull(lps->lvshared, idx))
> > + continue;
> > +
> > + lazy_vacuum_index(Irel[idx], [idx], vacrelstats->dead_tuples,
> > +   vacrelstats->old_live_tuples);
> > + }
> > +}
> >
> > In this function, if ParallelVacuumIsActive, we perform the parallel
> > vacuum for all the index for which parallel vacuum is supported and
> > once that is over we finish vacuuming remaining indexes for which
> > parallel vacuum is not supported.  But, my question is that inside
> > lazy_parallel_vacuum_or_cleanup_indexes, we wait for all the workers
> > to finish their job then only we start with the sequential vacuuming
> > shouldn't we start that immediately as soon as the leader
> > participation is over in the parallel vacuum?
>
> If we do that, while the leader process is vacuuming indexes that
> don't not support parallel vacuum sequentially some workers might be
> vacuuming for other indexes. Isn't it a problem?

I am not sure what could be the problem.

 If it's not problem,
> I think we can tie up indexes that don't support parallel vacuum to
> the leader and do parallel index vacuum.

I am not sure whether we can do that or not.  Because if we do a
parallel vacuum from the leader for the indexes which don't support a
parallel option then we will unnecessarily allocate the shared memory
for those indexes (index stats).  Moreover, I think it could also
cause a problem in a multi-pass vacuum if we try to copy its stats
into the shared memory.

I think simple option would be that as soon as leader participation is
over we can have a loop for all the indexes who don't support
parallelism in that phase and after completing that we wait for the
parallel workers to 

Re: Should we add xid_current() or a int8->xid cast?

[btfujiitkp]

On Tue, Oct 29, 2019 at 5:23 PM btfujiitkp  
wrote:

> Thomas Munro  writes:
>> On Sun, Sep 1, 2019 at 5:04 PM Thomas Munro 
>> wrote:
>>> Adding to CF.
>
>> Rebased.  An OID clashed so re-roll the dice.  Also spotted a typo.
>

I have some questions in this code.


Thanks for looking at the patch.


First,
"FullTransactionIdPrecedes(xmax, val)" is not equal to "val >= xmax" 
of

the previous code.  "FullTransactionIdPrecedes(xmax, val)" expresses
"val > xmax". Is it all right?

@@ -384,15 +324,17 @@ parse_snapshot(const char *str)
while (*str != '\0')
{
/* read next value */
-   val = str2txid(str, );
+   val = FullTransactionIdFromU64(pg_strtouint64(str, 
, 10));

str = endp;

/* require the input to be in order */
-   if (val < xmin || val >= xmax || val < last_val)
+   if (FullTransactionIdPrecedes(val, xmin) ||
+   FullTransactionIdPrecedes(xmax, val) ||
+   FullTransactionIdPrecedes(val, last_val))

In addition to it, as to current TransactionId(not FullTransactionId)
comparison, when we express ">=" of TransactionId, we use
"TransactionIdFollowsOrEquals". this method is referred by some 
methods.
On the other hand, FullTransactionIdFollowsOrEquals has not 
implemented

yet. So, how about implementing this method?


Good idea.  I added the missing variants:

+#define FullTransactionIdPrecedesOrEquals(a, b) ((a).value <= 
(b).value)

+#define FullTransactionIdFollows(a, b) ((a).value > (b).value)
+#define FullTransactionIdFollowsOrEquals(a, b) ((a).value >= 
(b).value)




Thank you for your patch.
It looks good.



Second,
About naming rule, "8" of xid8 means 8 bytes, but "8" has different
meaning in each situation. For example, int8 of PostgreSQL means 8
bytes, int8 of C language means 8 bits. If 64 is used, it just means 
64

bits. how about xid64()?


In C, the typenames use bits, by happy coincidence similar to the C99
stdint.h typenames (int32_t etc) that we should perhaps eventually
switch to.

In SQL, the types have names based on the number of bytes: int2, int4,
int8, float4, float8, not conforming to any standard but established
over 3 decades ago and also understood by a few other SQL systems.

That's unfortunate, but I can't see that ever changing.  I thought
that it would make most sense for the SQL type to be called xid8,
though admittedly it doesn't quite fit the pattern because xid is not
called xid4.  There is another example a bit like that: macaddr (6
bytes) and macaccdr8 (8 bytes).  As for the C type, we use
TransactionId and FullTransactionId (rather than, say, xid32 and
xid64).


That makes sense.

Anyway,
In the pg_proc.dat, "xid_snapshot_xip" should be "xid8_snapshot_xip".
And some parts of 0002 patch are rejected when I patch 0002 after 
patching 0001.


regards

Re: [HACKERS] Block level parallel vacuum

[Amit Kapila]

On Wed, Nov 13, 2019 at 11:39 AM Masahiko Sawada
 wrote:
>
> On Wed, 13 Nov 2019 at 12:43, Dilip Kumar  wrote:
> >
> >
> > In this function, if ParallelVacuumIsActive, we perform the parallel
> > vacuum for all the index for which parallel vacuum is supported and
> > once that is over we finish vacuuming remaining indexes for which
> > parallel vacuum is not supported.  But, my question is that inside
> > lazy_parallel_vacuum_or_cleanup_indexes, we wait for all the workers
> > to finish their job then only we start with the sequential vacuuming
> > shouldn't we start that immediately as soon as the leader
> > participation is over in the parallel vacuum?
>
> If we do that, while the leader process is vacuuming indexes that
> don't not support parallel vacuum sequentially some workers might be
> vacuuming for other indexes. Isn't it a problem?
>

Can you please explain what problem do you see with that?

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

Re: PHJ file leak.

[Amit Kapila]

On Wed, Nov 13, 2019 at 6:13 AM Kyotaro Horiguchi
 wrote:
>
> At Wed, 13 Nov 2019 09:48:19 +1300, Thomas Munro  
> wrote in
> >
> > Here's the version I'd like to commit in a day or two, once the dust
> > has settled on the minor release.  Instead of adding yet another copy
> > of that code, I just moved it out of the loop; this way there is no
> > way to miss it.  I think the comment could also be better, but I'll
> > wait for the concurrent discussions about the meaning of
> > ExecShutdownNode() to fix that in master.
>
> The phatch's shape looks better. Thanks.
>

+1.  LGTM as well.


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

Re: Coding in WalSndWaitForWal

[Amit Kapila]

On Wed, Nov 13, 2019 at 12:57 AM Andres Freund  wrote:
>
> On 2019-11-11 13:53:40 -0300, Alvaro Herrera wrote:
>
> >   /* Get a more recent flush pointer. */
> >   if (!RecoveryInProgress())
> >   RecentFlushPtr = GetFlushRecPtr();
> >   else
> >   RecentFlushPtr = GetXLogReplayRecPtr(NULL);
> >
> > + if (loc <= RecentFlushPtr)
> > + {
> > + SetLatch(MyLatch);
> > + return RecentFlushPtr;
> > + }
>
> Hm. I'm doubtful this is a good idea - it essentially means we'd not
> check for interrupts, protocol replies, etc. for an unbounded amount of
> time.
>

I think this function (WalSndWaitForWal) will be called from
WalSndLoop which checks for interrupts and protocol replies, so it
might not miss checking those things in that context.  In which case
it will miss to check those things for an unbounded amount of time?

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

Re: pg_waldump and PREPARE

[Fujii Masao]

On Wed, Nov 13, 2019 at 3:53 PM Andrey Lepikhov
 wrote:
>
>
>
> 12.11.2019 12:41, Fujii Masao пишет:
> > Ok, I changed the patch that way.
> > Attached is the latest version of the patch.
> >
> > Regards,
>
> I did not see any problems in this version of the patch. The information
> displayed by pg_waldump for the PREPARE record is sufficient for use.

Thanks Andrey and Michael for the review! I committed the patch.

Regards,

-- 
Fujii Masao

Re: Recovery performance of DROP DATABASE with many tablespaces

[Fujii Masao]

On Wed, Nov 13, 2019 at 3:57 PM k.jami...@fujitsu.com
 wrote:
>
> On Wed, Oct. 2, 2019 5:40 PM, Fujii Masao wrote:
> > On Tue, Jul 10, 2018 at 3:04 PM Michael Paquier  wrote:
> > >
> > > On Thu, Jul 05, 2018 at 01:42:20AM +0900, Fujii Masao wrote:
> > > > TBH, I have no numbers measured by the test.
> > > > One question about your test is; how did you measure the *recovery
> > > > time* of DROP DATABASE? Since it's *recovery* performance, basically
> > > > it's not easy to measure that.
> > >
> > > It would be simple to measure the time it takes to replay this single
> > > DROP DATABASE record by putting two gettimeofday() calls or such
> > > things and then take the time difference.  There are many methods that
> > > you could use here, and I suppose that with a shared buffer setting of
> > > a couple of GBs of shared buffers you would see a measurable
> > > difference with a dozen of tablespaces or so.  You could also take a
> > > base backup after creating all the tablespaces, connect the standby
> > > and then drop the database on the primary to see the actual time it
> > > takes.  Your patch looks logically correct to me because
> > > DropDatabaseBuffers is a
> > > *bottleneck* with large shared_buffers, and it would be nice to see
> > > numbers.
> >
> > Thanks for the comment!
> >
> > I measured how long it takes to replay DROP DATABASE with 1000 tablespaces,
> > in master and patched version. shared_buffers was set to 16GB.
> >
> > [master]
> > It took 8 seconds to replay DROP DATABASE with 1000 tablespaces, as follows.
> > In this case, 16GB shared_buffers was fully scanned 1000 times.
> >
> > 2019-10-02 16:50:14 JST LOG:  redo starts at 0/228
> > 2019-10-02 16:50:22 JST LOG:  redo done at 0/300A298
> >
> > [patched]
> > It took less than 1 second to replay DROP DATABASE with 1000 tablespaces,
> > as follows. In this case, 16GB shared_buffers was scanned only one time.
> >
> > 2019-10-02 16:47:03 JST LOG:  redo starts at 0/228
> > 2019-10-02 16:47:03 JST LOG:  redo done at 0/3001588
> >
>
> Hi Fujii-san,
>
> It's been a while, so I checked the patch once again.
> It's fairly straightforward and I saw no problems nor bug in the code.

Thanks for the review!

> > [patched]
> > It took less than 1 second to replay DROP DATABASE with 1000 tablespaces,
> The results are good.
> I want to replicate the performance to confirm the results as well.
> Could you share how you measured the recovery replay?

I forgot the actual steps that I used for the measurement.
But I think they are something like

1. create database "hoge"
2. create 1,000 tablespaces
3. create 1,000 tables on the database "hoge".
each table should be placed in different tablespace.
4. take a base backup
5. drop database "hoge"
6. shutdown the server with immediate mode
7. start an archive recovery from the backup taken at #4
8. measure how long it takes to apply DROP DATABASE record by
checking the timestamp at REDO start and REDO end.

I think that I performed the above steps on the master and
the patched version.

> Did you actually execute a failover?

No.

Regards,

-- 
Fujii Masao

Re: Coding in WalSndWaitForWal

[Kyotaro Horiguchi]

Ah, my stupid.

At Wed, 13 Nov 2019 16:34:49 +0900, Michael Paquier  wrote 
in 
> On Tue, Nov 12, 2019 at 11:27:16AM -0800, Andres Freund wrote:
> > It seems to me it'd be better to just remove the "get a more recent
> > flush pointer" block - it doesn't seem to currently surve a meaningful
> > purpose.
> 
> +1.  That was actually my suggestion upthread :)

Actually it is useless as it is. But the code still seems to me an
incomplete fast path (that lacks immediate return after it) for the
case where just one call to GetFlushRecPtr advances RecentFlushPtr is
enough.

However, I'm not confident taht removing the (intended) fast path
impacts perforance significantly. So I don't object to remove it.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: [HACKERS] Block level parallel vacuum

[Dilip Kumar]

On Wed, Nov 13, 2019 at 11:01 AM Amit Kapila  wrote:
>
> On Wed, Nov 13, 2019 at 9:48 AM Amit Kapila  wrote:
> >
> > Yeah, 0,2,3 and 4 sounds reasonable to me.  Earlier, Dilip also got
> > confused with option 1.
> >
>
> Let me try to summarize the discussion on this point and see if others
> have any opinion on this matter.
>
> We need a way to allow IndexAm to specify whether it can participate
> in a parallel vacuum.  As we know there are two phases of
> index-vacuum, bulkdelete and vacuumcleanup and in many cases, the
> bulkdelete performs the main deletion work and then vacuumcleanup just
> returns index statistics. So, for such cases, we don't want the second
> phase to be performed by a parallel vacuum worker.  Now, if the
> bulkdelete phase is not performed, then vacuumcleanup can process the
> entire index in which case it is better to do that phase via parallel
> worker.
>
> OTOH, in some cases vacuumcleanup takes another pass over-index to
> reclaim empty pages and update record the same in FSM even if
> bulkdelete is performed.  This happens in gin and bloom indexes.
> Then, we have an index where we do all the work in cleanup phase like
> in the case of brin indexes.  Now, for this category of indexes, we
> want vacuumcleanup phase to be also performed by a parallel worker.
>
> In short different indexes have different requirements for which phase
> of index vacuum can be performed in parallel.  Just to be clear, we
> can't perform both the phases (bulkdelete and cleanup) in one-go as
> bulk-delete can happen multiple times on a large index whereas
> vacuumcleanup is done once at the end.
>
> Based on these needs, we came up with a way to allow users to specify
> this information for IndexAm's. Basically, Indexam will expose a
> variable amparallelvacuumoptions which can have below options
>
> VACUUM_OPTION_NO_PARALLEL   1 << 0 # vacuum (neither bulkdelete nor
> vacuumcleanup) can't be performed in parallel
> VACUUM_OPTION_PARALLEL_BULKDEL   1 << 1 # bulkdelete can be done in
> parallel (Indexes nbtree, hash, gin, gist, spgist, bloom will set this
> flag)
> VACUUM_OPTION_PARALLEL_COND_CLEANUP  1 << 2 # vacuumcleanup can be
> done in parallel if bulkdelete is not performed (Indexes nbtree, brin,
> gin, gist,
> spgist, bloom will set this flag)
> VACUUM_OPTION_PARALLEL_CLEANUP  1 << 3 # vacuumcleanup can be done in
> parallel even if bulkdelete is already performed (Indexes gin, brin,
> and bloom will set this flag)
>
> We have discussed to expose this information via two variables but the
> above seems like a better idea to all the people involved.
>
> Any suggestions?  Anyone thinks this is not the right way to expose
> this information or there is no need to expose this information or
> they have a better idea for this?
>
> Sawada-San, Dilip, feel free to correct me.
Looks fine to me.


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

RE: Extension development

[Yonatan Misgan]

I have done the hard code. But my question is related to the concept how these 
extension components working together as a system. For example what the use 
case diagram looks like for my extension and also the other architectural view 
of the extension should look like.


Regards,

Yonathan Misgan
Assistant Lecturer, @ Debre Tabor University
Faculty of Technology
Department of Computer Science
Studying MSc in Computer Science (in Data and Web Engineering)
@ Addis Ababa University
E-mail: yona...@dtu.edu.et
yonathanmisga...@gmail.com
Tel:   (+251)-911180185 (mob)


From: Ahsan Hadi 
Sent: Tuesday, November 12, 2019 10:50:23 PM
To: Yonatan Misgan 
Cc: pgsql-hackers@lists.postgresql.org 
Subject: Re: Extension development

Hi Yonatan,

You can follow this blog for creating your own extension in PostgreSQL..

https://www.highgo.ca/2019/10/01/a-guide-to-create-user-defined-extension-modules-to-postgres/

-- Ahsan

On Tue, Nov 12, 2019 at 11:54 AM Yonatan Misgan 
mailto:yona...@dtu.edu.et>> wrote:

I am developed my own PostgreSQL extension for learning purpose and it is 
working correctly but I want to know to which components of the database is my 
own extension components communicate. For example I have c code, make file sql 
script, and control file after compiling the make file to which components of 
the database are each of my extension components to communicate. Thanks for 
your response.



Regards,



Yonathan Misgan

Assistant Lecturer, @ Debre Tabor University

Faculty of Technology

Department of Computer Science

Studying MSc in Computer Science (in Data and Web Engineering)

@ Addis Ababa University

E-mail: yona...@dtu.edu.et

yonathanmisga...@gmail.com

Tel:   (+251)-911180185 (mob)




--
Highgo Software (Canada/China/Pakistan)
URL : http://www.highgo.ca
ADDR: 10318 WHALLEY BLVD, Surrey, BC
EMAIL: mailto: ahsan.h...@highgo.ca

Extension development

[Yonatan Misgan]

Is there any one who help me what the architecture of an extension should looks 
like in PostgreSQL database.


Regards,

Yonathan Misgan
Assistant Lecturer, @ Debre Tabor University
Faculty of Technology
Department of Computer Science
Studying MSc in Computer Science (in Data and Web Engineering)
@ Addis Ababa University
E-mail: yona...@dtu.edu.et
yonathanmisga...@gmail.com
Tel:   (+251)-911180185 (mob)