Re: dropdb --force

[Pavel Stehule]

čt 14. 11. 2019 v 12:12 odesílatel vignesh C  napsal:

> On Wed, Nov 13, 2019 at 8:05 PM Pavel Stehule 
> wrote:
> >
> >
> >
> > 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
> >
>
> Thanks for working on the patch.
> Few minor comments:
> +Force termination of connected backends before removing the
> database.
> +   
> +   
> +This will add the FORCE option to the
> DROP
> +DATABASE command sent to the server.
> +   
>
> The above documentation can be changed similar to drop_database.sgml:
>  
>   Attempt to terminate all existing connections to the target database.
>   It doesn't terminate if prepared transactions, active logical
> replication
>   slots or subscriptions are present in the target database.
>  
>  
>   This will fail if the current user has no permissions to terminate
> other
>   connections. Required permissions are the same as with
>   pg_terminate_backend, described in
>   .  This will also fail if we
>   are not able to terminate connections.
>  
>
>
done


> We can make the modification in the same location as earlier in the below
> case:
> -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)" : "");
> +
>
>
done


> We can slightly rephrase the below:
> +printf(_("  -f, --force   force termination of
> connected backends\n"));
> can be changed to:
> +printf(_("  -f, --force   terminate the existing
> connections to the target database forcefully\n"));
>

the proposed text is too long - I changed the sentence, and any other can
fix it


> We can slightly rephrase the below:
> +/* now, only FORCE option can be used, so usage is very simple */
> +appendPQExpBuffer(, "DROP DATABASE %s%s%s;",
> can be changed to:
> +/* Generate drop db command using the options specified */
> +appendPQExpBuffer(, "DROP DATABASE %s%s%s;",
>

I would to say, so generating is simple due only one supported option. If
we support two or more options there, then the code should be more complex.
I changed comment to

/* Currently, only FORCE option is supported */

updated patch attached

Thank you for review

Pavel


> Regards,
> Vignesh
> EnterpriseDB: http://www.enterprisedb.com
>
diff --git a/doc/src/sgml/ref/dropdb.sgml b/doc/src/sgml/ref/dropdb.sgml
index 3fbdb33716..63b90005b4 100644
--- a/doc/src/sgml/ref/dropdb.sgml
+++ b/doc/src/sgml/ref/dropdb.sgml
@@ -86,6 +86,27 @@ PostgreSQL documentation
   
  
 
+ 
+  -f
+  --force
+  
+   
+Attempt to terminate all existing connections to the
+target database. It doesn't terminate if prepared
+transactions, active logical replication slots or
+subscriptions are present in the target database.
+   
+   
+This will fail if the current user has no permissions
+to terminate other connections. Required permissions
+are the same as with pg_terminate_backend,
+described in .
+This will also fail if we are not able to terminate
+connections.
+   
+  
+ 
+
  
-V
--version
diff --git a/src/bin/scripts/dropdb.c b/src/bin/scripts/dropdb.c
index dacd8e5f1d..ea7e5d8f75 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 = 

Re: segfault in geqo on experimental gcc animal

[Fabien COELHO]

Yep, I build periodically PostgreSQL package in openSUSE with the latest 
GCC and so that I identified that and isolated to a simple test-case. I 
would expect a fix today or tomorrow.


Indeed, the gcc issue reported seems fixed by gcc r278259. I'm updating 
moonjelly gcc to check if this solves pg compilation woes.


--
Fabien.

Re: Optimize partial TOAST decompression

[Rushabh Lathia]

On Thu, Nov 14, 2019 at 6:30 PM Tomas Vondra 
wrote:

> On Thu, Nov 14, 2019 at 03:27:42PM +0530, Rushabh Lathia wrote:
> >Today I noticed strange behaviour, consider the following test:
> >
> >postgres@126111=#create table foo ( a text );
> >CREATE TABLE
> >postgres@126111=#insert into foo values ( repeat('PostgreSQL is the
> >world''s best database and leading by an Open Source Community.', 8000));
> >INSERT 0 1
> >
> >postgres@126111=#select substring(a from 639921 for 81) from foo;
> > substring
> >---
> >
> >(1 row)
> >
>
> Hmmm. I think the issue is heap_tuple_untoast_attr_slice is using the
> wrong way to determine compressed size in the VARATT_IS_EXTERNAL_ONDISK
> branch. It does this
>
>  max_size = pglz_maximum_compressed_size(sliceoffset + slicelength,
>  TOAST_COMPRESS_SIZE(attr));
>
> But for the example you've posted TOAST_COMPRESS_SIZE(attr) returns 10,
> which is obviously bogus because the TOAST table contains ~75kB of data.
>
> I think it should be doing this instead:
>
>  max_size = pglz_maximum_compressed_size(sliceoffset + slicelength,
>  toast_pointer.va_extsize);
>
> At least that fixes it for me.
>
> I wonder if this actually explains the crashes 540f3168091 was supposed
> to fix, but it just masked them instead.
>


I tested the attached patch and that fixes the issue for me.

Thanks,


-- 
Rushabh Lathia
www.EnterpriseDB.com

Re: 'Invalid lp' during heap_xlog_delete

[Daniel Wood]

Sorry I missed one thing.  Turn off full page writes.  I'm running in an env. 
with atomic 8K writes.

> On November 12, 2019 at 6:23 PM Daniel Wood  wrote:
> 
> It's been tedious to get it exactly right but I think I got it.  FYI, I 
> was delayed because today we had yet another customer hit this:  'redo max 
> offset' error.  The system crashed as a number of autovacuums and a 
> checkpoint happened and then the REDO failure
> 

Re: Hypothetical indexes using BRIN broken since pg10

[Michael Paquier]

On Wed, Sep 25, 2019 at 07:03:52AM +0200, Julien Rouhaud wrote:
> IIUC, if something like Heikki's patch is applied on older branch the
> problem will be magically fixed from the extension point of view so
> that should be safe (an extension would only need to detect the minor
> version to get a more useful error message for users), and all
> alternatives are too intrusive to be patckbatched.

So, Heikki, are you planning to work more on that and commit a change
close to what has been proposed upthread in [1]?  It sounds to me that
this has the advantage to be non-intrusive and a similar solution has
been used for GIN indexes.  Moving the redesign out of the discussion,
is there actually a downsize with back-patching something like
Heikki's version?

Tom, Alvaro and Julien, do you have more thoughts to share?

[1]: 
https://www.postgresql.org/message-id/b847493e-d263-3f2e-1802-689e778c9...@iki.fi
--
Michael


signature.asc
Description: PGP signature

Re: cost based vacuum (parallel)

[Amit Kapila]

On Wed, Nov 13, 2019 at 10:02 AM Masahiko Sawada
 wrote:
>
> I've done some tests while changing shared buffer size, delays and
> number of workers. The overall results has the similar tendency as the
> result shared by Dilip and looks reasonable to me.
>

Thanks, Sawada-san for repeating the tests.  I can see from yours,
Dilip and Mahendra's testing that the delay is distributed depending
upon the I/O done by a particular worker and the total I/O is also as
expected in various kinds of scenarios.  So, I think this is a better
approach.  Do you agree or you think we should still investigate more
on another approach as well?

I would like to summarize this approach.  The basic idea for parallel
vacuum is to allow the parallel workers and master backend to have a
shared view of vacuum cost related parameters (mainly
VacuumCostBalance) and allow each worker to update it and then based
on that decide whether it needs to sleep.  With this basic idea, we
found that in some cases the throttling is not accurate as explained
with an example in my email above [1] and then the tests performed by
Dilip and others in the following emails (In short, the workers doing
more I/O can be throttled less).  Then as discussed in an email later
[2], we tried a way to avoid letting the workers sleep which has done
less or no I/O as compared to other workers.  This ensured that
workers who are doing more I/O got throttled more.  The idea is to
allow any worker to sleep only if it has performed the I/O above a
certain threshold and the overall balance is more than the cost_limit
set by the system.  Then we will allow the worker to sleep
proportional to the work done by it and reduce the
VacuumSharedCostBalance by the amount which is consumed by the current
worker.  This scheme leads to the desired throttling by different
workers based on the work done by the individual worker.

We have tested this idea with various kinds of workloads like by
varying shared buffer size, delays and number of workers.  Then also,
we have tried with a different number of indexes and workers.  In all
the tests, we found that the workers are throttled proportional to the
I/O being done by a particular worker.


[1] - 
https://www.postgresql.org/message-id/CAA4eK1JvxBTWTPqHGx1X7in7j42ZYwuKOZUySzH3YMwTNRE-2Q%40mail.gmail.com
[2] - 
https://www.postgresql.org/message-id/CAA4eK1K9kCqLKbVA9KUuuarjj%2BsNYqrmf6UAFok5VTgZ8evWoA%40mail.gmail.com

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

Re: Replication & recovery_min_apply_delay

[Michael Paquier]

On Tue, Sep 10, 2019 at 03:23:25PM +0900, Michael Paquier wrote:
> Yes, I suspect that it could be very costly in some configurations if
> there is a large gap between the last replayed LSN and the last LSN
> the WAL receiver has flushed.
> 
> There is an extra thing, which has not been mentioned yet on this
> thread, that we need to be very careful about:
>
>When the standby is started and primary_conninfo is 
> set
>correctly, the standby will connect to the primary after replaying all
>WAL files available in the archive. If the connection is established
>successfully, you will see a walreceiver process in the standby, and
>a corresponding walsender process in the primary.
> 
> This is a long-standing behavior, and based on the first patch
> proposed we would start a WAL receiver once consistency has been
> reached if there is any delay defined even if restore_command is
> enabled.  We cannot assume either that everybody will want to start a
> WAL receiver in this configuration if there is archiving behind with a
> lot of segments which allow for a larger catchup window..

Two months later, we still have a patch registered in the CF which is
incorrect on a couple of aspects, and with scenarios which are
documented and getting broken.  Shouldn't we actually have a GUC to
control the startup of the WAL receiver instead?
--
Michael


signature.asc
Description: PGP signature

Re: could not stat promote trigger file leads to shutdown

[Fujii Masao]

On Fri, Nov 15, 2019 at 10:49 AM Michael Paquier  wrote:
>
> On Thu, Nov 14, 2019 at 10:38:30AM -0500, Tom Lane wrote:
> > It is harsh, but I suspect if we just logged the complaint, we'd get
> > bug reports about "Postgres isn't reacting to my trigger file",
> > because people don't read the postmaster log unless forced to.
> > Is there some more-visible way to report the problem, short of
> > shutting down?
> >
> > (BTW, from this perspective, WARNING is especially bad because it
> > might not get logged at all.  Better to use LOG.)
>
> Neither am I comfortable with that.

I always wonder why WARNING was defined that way.
I think that users usually pay attention to the word "WARNING"
rather than "LOG".

> > One thought is to try to detect the misconfiguration at postmaster
> > start --- better to fail at startup than sometime later.  But I'm
> > not sure how reliably we could do that.
>
> I think that we could do something close to the area where
> RemovePromoteSignalFiles() gets called.  Why not simply checking the
> path defined by PromoteTriggerFile() at startup time then?  I take it
> that the only thing we should not complain about is stat() returning
> ENOENT when looking at the promote file defined.

promote_trigger_file is declared as PGC_SIGHUP,
so such check would be necessary even while the standby is running.
Which can cause the server to fail after the startup.

Regards,

-- 
Fujii Masao

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

[Michael Paquier]

On Wed, Nov 13, 2019 at 05:55:12PM -0800, Andres Freund wrote:
> 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.

FWIW, I am reading the same, aka the sentence of Daniel is wrong.  And
that what you say is right.

> 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?

[ ... Looking ... ]
You are right, we could just rely on cross-checking that when we have
no data then XLH_INSERT_CONTAINS_NEW_TUPLE is not set, or something
like that:
@@ -901,11 +901,17 @@ DecodeMultiInsert(LogicalDecodingContext *ctx,
XLogRecordBuffer *buf)
return;

/*
-* As multi_insert is not used for catalogs yet, the block should always
-* have data even if a full-page write of it is taken.
+* multi_insert can be used by catalogs, hence it is possible that
+* the block does not have any data even if a full-page write of it
+* is taken.
 */
 tupledata = XLogRecGetBlockData(r, 0, );
-Assert(tupledata != NULL);
+Assert(tupledata == NULL ||
+   (xlrec->flags & XLH_INSERT_CONTAINS_NEW_TUPLE) != 0);
+
+/* No data, then leave */
+if (tupledata == NULL)
+return;

The latest patch does not apply, so it needs a rebase.
--
Michael


signature.asc
Description: PGP signature

Re: could not stat promote trigger file leads to shutdown

[Michael Paquier]

On Thu, Nov 14, 2019 at 10:38:30AM -0500, Tom Lane wrote:
> It is harsh, but I suspect if we just logged the complaint, we'd get
> bug reports about "Postgres isn't reacting to my trigger file",
> because people don't read the postmaster log unless forced to.
> Is there some more-visible way to report the problem, short of
> shutting down?
> 
> (BTW, from this perspective, WARNING is especially bad because it
> might not get logged at all.  Better to use LOG.)

Neither am I comfortable with that.

> One thought is to try to detect the misconfiguration at postmaster
> start --- better to fail at startup than sometime later.  But I'm
> not sure how reliably we could do that.

I think that we could do something close to the area where
RemovePromoteSignalFiles() gets called.  Why not simply checking the
path defined by PromoteTriggerFile() at startup time then?  I take it
that the only thing we should not complain about is stat() returning
ENOENT when looking at the promote file defined.
--
Michael


signature.asc
Description: PGP signature

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

[Michael Paquier]

On Thu, Nov 14, 2019 at 11:20:25AM +0300, Nikolay Shaplov wrote:
> For me there is no mush sense in it, as it does not prevent us from wrong 
> type 
> casting. Indexes can use all kinds of structures for reloptions, and checking 
> that this is index, will not do much.

It seems to me that if the plan is to have one option structure for
each index AM, which has actually the advantage to reduce the bloat of
each relcache entry currently relying on StdRdOptions, then we could
have those extra assertion checks in the same patch, because the new
macros are introduced.

> Do you know way how to distinguish one index from another? If we can check in 
> assertion this is index, and this index is spgist, then assertion will make 
> sense for 100%. I just have no idea how to do it. As far as I can see it is 
> not possible now.

There is rd_rel->relam.  You can for example refer to pgstatindex.c
which has AM-related checks to make sure that the correct index AM is
being used.
--
Michael


signature.asc
Description: PGP signature

Re: SKIP_LOCKED test causes random buildfarm failures

[Michael Paquier]

On Thu, Nov 14, 2019 at 03:20:09PM -0500, Tom Lane wrote:
> If we're going to keep them in vacuum.sql, we should use the
> client_min_messages fix there, as that's a full solution not just
> reducing the window.  But I don't agree that these tests are worth
> the cycles, given the coverage elsewhere.  The probability of breaking
> this option is just not high enough to justify core-regression-test
> coverage.

I would rather keep the solution with client_min_messages, and the
tests in vacuum.sql to keep those checks for the grammar parsing.  So
this basically brings us back to use the patch I proposed here:
https://www.postgresql.org/message-id/20191107013942.ga1...@paquier.xyz

Any objections?
--
Michael


signature.asc
Description: PGP signature

RE: Planning counters in pg_stat_statements (using pgss_store)

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

On Wed, Nov 13, 2019 at 10:50 AM, Julien Rouhaud wrote:
> 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.

Okay. It's not much a serious problem, so we can leave it as it is.


> > > 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.

Storing buffer usage to different columns is useful to detect the cause of the 
problems if there are the cases many buffers are used during planning, but I'm 
also unsure those cases actually exist. 


> > 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.

Actually I also don't have strong opinion but I thought someone would complain 
about renaming of those columns and also some tools like monitoring which use 
those columns will not work. If we use {total, min, max, mean, stddev}_time, 
someone might mistakenly understand {total, min, max, mean, stddev}_time mean 
{total, min, max, mean, stddev} of planning and execution. 
If I need to choose {total, min, max, mean, stddev}_time or {total, min, max, 
mean, stddev}_exec_time, I choose former one because choosing best name is not 
worth destructing the existing scripts or tools.

Thanks.
--
Yoshikazu Imai 

Re: format of pg_upgrade loadable_libraries warning

[Daniel Gustafsson]

> On 14 Nov 2019, at 23:16, Bruce Momjian  wrote:
> 
> On Thu, Nov 14, 2019 at 04:06:52PM -0300, Alvaro Herrera wrote:
>> BTW, how is one supposed to "manually upgrade databases that use
>> contrib/isb"?  This part is not very clear.
> 
> I thought you would dump out databases that use isn, drop those
> databases, use pg_upgrade for the remaining databases, then load the
> dumped database.  Attached is a patch that improves the wording.

I agree with this patch, that's much a more informative message.

There is one tiny typo in the patch: s/laster/later/

+"cluster, drop them, restart the upgrade, and 
restore them laster.  A\n"

cheers ./daniel

Re: Missing dependency tracking for TableFunc nodes

[Tomas Vondra]

On Thu, Nov 14, 2019 at 05:35:06PM -0500, Tom Lane wrote:

Tomas Vondra  writes:

On Thu, Nov 14, 2019 at 04:36:54PM -0500, Tom Lane wrote:

Hm.  No, it's not enough, unless you add more logic to deal with the
possibility that the stats object is gone by the time you have the
table lock.  Consider e.g. two concurrent DROP STATISTICS commands,
or a statistics drop that's cascading from something like a drop
of a relevant function and so has no earlier table lock.



Isn't that solved by RemoveObjects() doing this?



/* Get an ObjectAddress for the object. */
address = get_object_address(stmt->removeType,
 object,
 ,
 AccessExclusiveLock,
 stmt->missing_ok);


Ah, I see, we already have AEL on the stats object itself.  So that
eliminates my concern about a race between two RemoveStatisticsById
calls, but what we have instead is fear of deadlock.  A DROP STATISTICS
command will acquire AEL on the stats object but then AEL on the table,
the opposite of what will happen during DROP TABLE, so concurrent
executions of those will deadlock.  That might be better than the
failures Mark is seeing now, but not by much.



Hmmm, yeah :-(


A correct fix I think is that the planner ought to acquire AccessShareLock
on a stats object it's trying to use (and then recheck whether the object
is still there).  That seems rather expensive, but there may be no other
way.


Yes, so something like for indexes, although we don't need the recheck
in that case. I think the attached patch does that (but it's 1AM here).

regards

--
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services 
diff --git a/src/backend/optimizer/util/plancat.c 
b/src/backend/optimizer/util/plancat.c
index 5e889d1861..a1e325615c 100644
--- a/src/backend/optimizer/util/plancat.c
+++ b/src/backend/optimizer/util/plancat.c
@@ -46,6 +46,7 @@
 #include "rewrite/rewriteManip.h"
 #include "statistics/statistics.h"
 #include "storage/bufmgr.h"
+#include "storage/lmgr.h"
 #include "utils/builtins.h"
 #include "utils/lsyscache.h"
 #include "utils/partcache.h"
@@ -1305,9 +1306,13 @@ get_relation_statistics(RelOptInfo *rel, Relation 
relation)
Bitmapset  *keys = NULL;
int i;
 
+   LockDatabaseObject(StatisticExtRelationId, statOid, 0, 
AccessShareLock);
+
+   /* we may get invalid tuple, if the statistic just got dropped 
*/
htup = SearchSysCache1(STATEXTOID, ObjectIdGetDatum(statOid));
if (!HeapTupleIsValid(htup))
-   elog(ERROR, "cache lookup failed for statistics object 
%u", statOid);
+   continue;
+
staForm = (Form_pg_statistic_ext) GETSTRUCT(htup);
 
dtup = SearchSysCache1(STATEXTDATASTXOID, 
ObjectIdGetDatum(statOid));

Re: Missing dependency tracking for TableFunc nodes

[Tomas Vondra]

On Thu, Nov 14, 2019 at 07:27:29PM -0300, Alvaro Herrera wrote:

On 2019-Nov-14, Tomas Vondra wrote:


Isn't that solved by RemoveObjects() doing this?

   /* Get an ObjectAddress for the object. */
   address = get_object_address(stmt->removeType,
object,
,
AccessExclusiveLock,
stmt->missing_ok);

I've actually done some debugging before sending the patch, and I think
this prevent the issue you describe.


Hmm .. shouldn't get_statistics_object_oid get a lock on the table that
owns the stats object too?  I think it should be setting *relp to it.
That way, the lock you're proposing to add would be obtained there.
That means it'd be similar to what we do for OBJECT_TRIGGER etc,
get_object_address_relobject().



Hmmm, maybe. We'd have to fake the list of names, because that function
expects the relation name to be included in the list of names, and we
don't have that for extended stats. But it might work, I guess.


I admit this'd crash and burn if we had stats on multiple relations,
because there'd be no way to return the multiple relations that would
end up locked.



I think that's less important now. If we ever get that feature, we'll
need to make that work somehow.


regards

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

Re: format of pg_upgrade loadable_libraries warning

[Bruce Momjian]

On Thu, Nov 14, 2019 at 05:49:12PM -0500, Tom Lane wrote:
> Bruce Momjian  writes:
> > On Thu, Nov 14, 2019 at 04:06:52PM -0300, Alvaro Herrera wrote:
> >> BTW, how is one supposed to "manually upgrade databases that use
> >> contrib/isb"?  This part is not very clear.
> 
> > I thought you would dump out databases that use isn, drop those
> > databases, use pg_upgrade for the remaining databases, then load the
> > dumped database.  Attached is a patch that improves the wording.
> 
> That's better wording, but do we need similar for any of the other
> not-supported checks?

I don't think so.  The other checks are checking for the _use_ of
certain things in user objects, and the user objects can be dropped,
while this check checks for the existence of _functions_ from an
extension that must be uninstalled.  I assume the extension can be
uninstalled in the database, but I assume something uses it and that
telling people to find all the users of the extension then dropping it
is too complex to describe.

-- 
  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: format of pg_upgrade loadable_libraries warning

[Tom Lane]

Bruce Momjian  writes:
> On Thu, Nov 14, 2019 at 04:06:52PM -0300, Alvaro Herrera wrote:
>> BTW, how is one supposed to "manually upgrade databases that use
>> contrib/isb"?  This part is not very clear.

> I thought you would dump out databases that use isn, drop those
> databases, use pg_upgrade for the remaining databases, then load the
> dumped database.  Attached is a patch that improves the wording.

That's better wording, but do we need similar for any of the other
not-supported checks?

regards, tom lane

Re: Missing dependency tracking for TableFunc nodes

[Tom Lane]

Tomas Vondra  writes:
> On Thu, Nov 14, 2019 at 04:36:54PM -0500, Tom Lane wrote:
>> Hm.  No, it's not enough, unless you add more logic to deal with the
>> possibility that the stats object is gone by the time you have the
>> table lock.  Consider e.g. two concurrent DROP STATISTICS commands,
>> or a statistics drop that's cascading from something like a drop
>> of a relevant function and so has no earlier table lock.

> Isn't that solved by RemoveObjects() doing this?

> /* Get an ObjectAddress for the object. */
> address = get_object_address(stmt->removeType,
>  object,
>  ,
>  AccessExclusiveLock,
>  stmt->missing_ok);

Ah, I see, we already have AEL on the stats object itself.  So that
eliminates my concern about a race between two RemoveStatisticsById
calls, but what we have instead is fear of deadlock.  A DROP STATISTICS
command will acquire AEL on the stats object but then AEL on the table,
the opposite of what will happen during DROP TABLE, so concurrent
executions of those will deadlock.  That might be better than the
failures Mark is seeing now, but not by much.

A correct fix I think is that the planner ought to acquire AccessShareLock
on a stats object it's trying to use (and then recheck whether the object
is still there).  That seems rather expensive, but there may be no other
way.

regards, tom lane

Re: Missing dependency tracking for TableFunc nodes

[Alvaro Herrera]

On 2019-Nov-14, Tomas Vondra wrote:

> Isn't that solved by RemoveObjects() doing this?
> 
>/* Get an ObjectAddress for the object. */
>address = get_object_address(stmt->removeType,
> object,
> ,
> AccessExclusiveLock,
> stmt->missing_ok);
> 
> I've actually done some debugging before sending the patch, and I think
> this prevent the issue you describe.

Hmm .. shouldn't get_statistics_object_oid get a lock on the table that
owns the stats object too?  I think it should be setting *relp to it.
That way, the lock you're proposing to add would be obtained there.
That means it'd be similar to what we do for OBJECT_TRIGGER etc,
get_object_address_relobject().

I admit this'd crash and burn if we had stats on multiple relations,
because there'd be no way to return the multiple relations that would
end up locked.

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

Re: Missing dependency tracking for TableFunc nodes

[Tomas Vondra]

On Thu, Nov 14, 2019 at 04:36:54PM -0500, Tom Lane wrote:

Tomas Vondra  writes:

On Wed, Nov 13, 2019 at 08:37:59PM -0500, Tom Lane wrote:

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 :-(



I think the right thing to do is simply acquire AE lock on the relation
in RemoveStatisticsById, per the attached patch. It's possible we'll
need to do something more complicated once join stats are added, but
for now this should be enough (and backpatchable).


Hm.  No, it's not enough, unless you add more logic to deal with the
possibility that the stats object is gone by the time you have the
table lock.  Consider e.g. two concurrent DROP STATISTICS commands,
or a statistics drop that's cascading from something like a drop
of a relevant function and so has no earlier table lock.



Isn't that solved by RemoveObjects() doing this?

   /* Get an ObjectAddress for the object. */
   address = get_object_address(stmt->removeType,
object,
,
AccessExclusiveLock,
stmt->missing_ok);

I've actually done some debugging before sending the patch, and I think
this prevent the issue you describe.


regards

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

Re: format of pg_upgrade loadable_libraries warning

[Bruce Momjian]

On Thu, Nov 14, 2019 at 02:46:29PM -0500, Tom Lane wrote:
> Alvaro Herrera  writes:
> > On 2019-Oct-07, Bruce Momjian wrote:
> >> and the "in database" (which I have changed to capitalized "In database"
> >> in the attached patch), looks like:
> >> fprintf(script, "In database: %s\n", active_db->db_name);
> >> meaning it _isn't_ an output error message, but rather something that
> >> appears in an error file.  I don't think either of these are translated.
> >> Is that wrong?
> 
> > pg_fatal is a "gettext trigger" (see nls.mk), so that part of the
> > message is definitely translated.
> 
> Right, but Bruce's point is that what goes into the separate output
> file listing problem cases is not translated, and never has been.
> Maybe we should start doing so, but that would be a distinct issue.
> I'm not really sure that we should translate it, anyway --- could
> there be anyone out there who is using tools to process these files?

Yes, we are lacking in all these output files so if we do one, we should
do them all.

-- 
  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: format of pg_upgrade loadable_libraries warning

[Bruce Momjian]

On Thu, Nov 14, 2019 at 04:06:52PM -0300, Alvaro Herrera wrote:
> BTW, how is one supposed to "manually upgrade databases that use
> contrib/isb"?  This part is not very clear.

I thought you would dump out databases that use isn, drop those
databases, use pg_upgrade for the remaining databases, then load the
dumped database.  Attached is a patch that improves the wording.

-- 
  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 +
diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c
index ff7057db73..e5f1c64799 100644
--- a/src/bin/pg_upgrade/check.c
+++ b/src/bin/pg_upgrade/check.c
@@ -887,9 +887,9 @@ check_for_isn_and_int8_passing_mismatch(ClusterInfo *cluster)
 		pg_fatal("Your installation contains \"contrib/isn\" functions which rely on the\n"
  "bigint data type.  Your old and new clusters pass bigint values\n"
  "differently so this cluster cannot currently be upgraded.  You can\n"
- "manually upgrade databases that use \"contrib/isn\" facilities and remove\n"
- "\"contrib/isn\" from the old cluster and restart the upgrade.  A list of\n"
- "the problem functions is in the file:\n"
+ "manually dump databases that use \"contrib/isn\" facilities in the old\n"
+ "cluster, drop them, restart the upgrade, and restore them laster.  A\n"
+ "list of the problem functions is in the file:\n"
  "%s\n\n", output_path);
 	}
 	else

Re: Using multiple extended statistics for estimates

[Tomas Vondra]

On Thu, Nov 14, 2019 at 01:17:02PM -0800, Mark Dilger wrote:



On 11/14/19 12:04 PM, Tomas Vondra wrote:

Aha, I think I understand now - thanks for the explanation. You're right
the comment is trying to explain why just taking the last clause for a
given attnum is fine. I'll try to make the comment clearer.

For the case with equal Const values that should be mostly obvious, i.e.
"a=1 AND a=1 AND a=1" has the same selectivity as "a=1".

The case with different Const values is harder, unfortunately. It might
seem obvious that "a=1 AND a=2" means there are no matching rows, but
that heavily relies on the semantics of the equality operator. And we
can't simply compare the Const values either, I'm afraid, because there
are cases with cross-type operators like

 a = 1::int AND a = 1.0::numeric

where the Consts are of different type, yet both conditions can be true.

So it would be pretty tricky to do this, and the current code does not
even try to do that.

Instead, it just assumes that it's mostly fine to overestimate, because
then at runtime we'll simply end up with 0 rows here.


I'm unsure whether that could be a performance problem at runtime.

I could imagine the planner short-circuiting additional planning when
it finds a plan with zero rows, and so we'd save planner time if we
avoid overestimating.  I don't recall if the planner does anything like
that, or if there are plans to implement such logic, but it might be
good not to rule it out.  Tom's suggestion elsewhere in this thread to
use code in predtest.c sounds good to me.



No, AFAIK the planner does not do anything like that - it might probaly
do that if it could prove there are no such rows, but that's hardly the
case for estimates based on approximate information (i.e. statistics).

If could do that based on the predicate analysis in predtest.c mentioned
by Tom, although I don't think it does anything beyond tweaking the row
estimate to ~1 row.


I don't know if you want to expand the scope of this particular patch to
include that, though.



Certainly not. It's an interesting but surprisingly complicated problem,
and this patch simply aims to add different improvement.

regards

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

Re: Using multiple extended statistics for estimates

[Tomas Vondra]

On Thu, Nov 14, 2019 at 03:16:04PM -0500, Tom Lane wrote:

Tomas Vondra  writes:

For the case with equal Const values that should be mostly obvious, i.e.
"a=1 AND a=1 AND a=1" has the same selectivity as "a=1".



The case with different Const values is harder, unfortunately. It might
seem obvious that "a=1 AND a=2" means there are no matching rows, but
that heavily relies on the semantics of the equality operator. And we
can't simply compare the Const values either, I'm afraid, because there
are cases with cross-type operators like
  a = 1::int AND a = 1.0::numeric
where the Consts are of different type, yet both conditions can be true.


FWIW, there's code in predtest.c to handle exactly that, at least for
types sharing a btree opfamily.  Whether it's worth applying that logic
here is unclear, but note that we've had the ability to recognize
redundant and contradictory clauses for a long time:

regression=# explain select * from tenk1 where two = 1;
QUERY PLAN

Seq Scan on tenk1  (cost=0.00..470.00 rows=5000 width=244)
  Filter: (two = 1)
(2 rows)

regression=# explain select * from tenk1 where two = 1 and two = 1::bigint;
QUERY PLAN

Seq Scan on tenk1  (cost=0.00..470.00 rows=5000 width=244)
  Filter: (two = 1)
(2 rows)

regression=# explain select * from tenk1 where two = 1 and two = 2::bigint;
 QUERY PLAN
---
Result  (cost=0.00..470.00 rows=1 width=244)
  One-Time Filter: false
  ->  Seq Scan on tenk1  (cost=0.00..470.00 rows=1 width=244)
Filter: (two = 1)
(4 rows)

It falls down on

regression=# explain select * from tenk1 where two = 1 and two = 2::numeric;
   QUERY PLAN
---
Seq Scan on tenk1  (cost=0.00..520.00 rows=25 width=244)
  Filter: ((two = 1) AND ((two)::numeric = '2'::numeric))
(2 rows)

because numeric isn't in the same opfamily, so these clauses can't be
compared easily.

regards, tom lane


Yeah, and this logic still works - the redundant clauses won't even get
to the selectivity estimation, I think. So maybe the comment is not
quite necessary, because the problem does not even exist ...

Maybe we could do something about the cases that predtest.c can't solve,
but it's not clear if we can be much smarter for types with different
opfamilies.

regards

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

Re: Missing dependency tracking for TableFunc nodes

[Tom Lane]

Tomas Vondra  writes:
> On Wed, Nov 13, 2019 at 08:37:59PM -0500, Tom Lane wrote:
>> 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 :-(

> I think the right thing to do is simply acquire AE lock on the relation
> in RemoveStatisticsById, per the attached patch. It's possible we'll
> need to do something more complicated once join stats are added, but
> for now this should be enough (and backpatchable).

Hm.  No, it's not enough, unless you add more logic to deal with the
possibility that the stats object is gone by the time you have the
table lock.  Consider e.g. two concurrent DROP STATISTICS commands,
or a statistics drop that's cascading from something like a drop
of a relevant function and so has no earlier table lock.

regards, tom lane

Re: Missing dependency tracking for TableFunc nodes

[Tomas Vondra]

On Wed, Nov 13, 2019 at 08:37:59PM -0500, Tom Lane wrote:

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 :-(



I think the right thing to do is simply acquire AE lock on the relation
in RemoveStatisticsById, per the attached patch. It's possible we'll
need to do something more complicated once join stats are added, but
for now this should be enough (and backpatchable).

regards

--
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services 
diff --git a/src/backend/commands/statscmds.c b/src/backend/commands/statscmds.c
index 0ab43deb71..1126e7a585 100644
--- a/src/backend/commands/statscmds.c
+++ b/src/backend/commands/statscmds.c
@@ -30,6 +30,7 @@
 #include "commands/defrem.h"
 #include "miscadmin.h"
 #include "statistics/statistics.h"
+#include "storage/lmgr.h"
 #include "utils/builtins.h"
 #include "utils/fmgroids.h"
 #include "utils/inval.h"
@@ -533,6 +534,21 @@ RemoveStatisticsById(Oid statsOid)
Form_pg_statistic_ext statext;
Oid relid;
 
+   /* Lookup and lock relation the statistics is defined on. */
+   tup = SearchSysCache1(STATEXTOID, ObjectIdGetDatum(statsOid));
+
+   if (!HeapTupleIsValid(tup))
+   elog(ERROR, "cache lookup failed for statistics object %u", 
statsOid);
+
+   statext = (Form_pg_statistic_ext) GETSTRUCT(tup);
+   relid = statext->stxrelid;
+
+   /* lock the relation in access exclusive mode */
+   LockRelationOid(relid, AccessExclusiveLock);
+
+   ReleaseSysCache(tup);
+
+
/*
 * First delete the pg_statistic_ext_data tuple holding the actual
 * statistical data.

Re: [PATCH] Implement INSERT SET syntax

[Tom Lane]

Gareth Palmer  writes:
>> On 19/08/2019, at 3:00 AM, Tom Lane  wrote:
>> Perhaps the way to resolve Peter's objection is to make the syntax
>> more fully like UPDATE:
>> INSERT INTO target SET c1 = x, c2 = y+z, ... FROM tables-providing-x-y-z
>> (with the patch as-submitted corresponding to the case with an empty
>> FROM clause, hence no variables in the expressions-to-be-assigned).

> Thanks for the feedback. Attached is version 3 of the patch that makes
> the syntax work more like an UPDATE statement when a FROM clause is used.

Since nobody has objected to this, I'm supposing that there's general
consensus that that design sketch is OK, and we can move on to critiquing
implementation details.  I took a look, and didn't like much of what I saw.

* In the grammar, there's no real need to have separate productions
for the cases with FROM and without.  The way you have it is awkward,
and it arbitrarily rejects combinations that work fine in plain
SELECT, such as WHERE without FROM.  You should just do

insert_set_clause:
SET set_clause_list from_clause where_clause
  group_clause having_clause window_clause opt_sort_clause
  opt_select_limit

relying on the ability of all those symbols (except set_clause_list) to
reduce to empty.

* This is randomly inconsistent with select_no_parens, and not in a
good way, because you've omitted the option that's actually most likely
to be useful, namely for_locking_clause.  I wonder whether it's practical
to refactor select_no_parens so that the stuff involving optional trailing
clauses can be separated out into a production that insert_set_clause
could also use.  Might not be worth the trouble, but I'm concerned
about select_no_parens growing additional clauses that we then forget
to also add to insert_set_clause.

* I'm not sure if it's worth also refactoring simple_select so that
the "into_clause ... window_clause" business could be shared.  But
it'd likely be a good idea to at least have a comment there noting
that any changes in that production might need to be applied to
insert_set_clause as well.

* In kind of the same vein, it feels like the syntax documentation
is awkwardly failing to share commonality that it ought to be
able to share with the SELECT man page.

* I dislike the random hacking you did in transformMultiAssignRef.
That weakens a useful check for error cases, and it's far from clear
why the new assertion is OK.  It also raises the question of whether
this is really the only place you need to touch in parse analysis.
Perhaps it'd be better to consider inventing new EXPR_KIND_ values
for this situation; you'd then have to run around and look at all the
existing EXPR_KIND uses, but that seems like a useful cross-check
activity anyway.  Or maybe we need to take two steps back and
understand why that change is needed at all.  I'd imagined that this
patch would be only syntactic sugar for something you can do already,
so it's not quite clear to me why we need additional changes.

(If it's *not* just syntactic sugar, then the scope of potential
problems becomes far greater, eg does ruleutils.c need to know
how to reconstruct a valid SQL command from a querytree like this.
If we're not touching ruleutils.c, we need to be sure that every
command that can be written this way can be written old-style.)

* Other documentation gripes: the lone example seems insufficient,
and there needs to be an entry under COMPATIBILITY pointing out
that this is not per SQL spec.

* Some of the test cases seem to be expensively repeating
construction/destruction of tables that they could have shared with
existing test cases.  I do not consider it a virtue for new tests
added to an existing test script to be resolutely independent of
what's already in that script.

I'm setting this back to Waiting on Author.

regards, tom lane

Re: Using multiple extended statistics for estimates

[Mark Dilger]

On 11/14/19 12:04 PM, Tomas Vondra wrote:

Aha, I think I understand now - thanks for the explanation. You're right
the comment is trying to explain why just taking the last clause for a
given attnum is fine. I'll try to make the comment clearer.

For the case with equal Const values that should be mostly obvious, i.e.
"a=1 AND a=1 AND a=1" has the same selectivity as "a=1".

The case with different Const values is harder, unfortunately. It might
seem obvious that "a=1 AND a=2" means there are no matching rows, but
that heavily relies on the semantics of the equality operator. And we
can't simply compare the Const values either, I'm afraid, because there
are cases with cross-type operators like

  a = 1::int AND a = 1.0::numeric

where the Consts are of different type, yet both conditions can be true.

So it would be pretty tricky to do this, and the current code does not
even try to do that.

Instead, it just assumes that it's mostly fine to overestimate, because
then at runtime we'll simply end up with 0 rows here.


I'm unsure whether that could be a performance problem at runtime.

I could imagine the planner short-circuiting additional planning when
it finds a plan with zero rows, and so we'd save planner time if we
avoid overestimating.  I don't recall if the planner does anything like
that, or if there are plans to implement such logic, but it might be
good not to rule it out.  Tom's suggestion elsewhere in this thread to
use code in predtest.c sounds good to me.

I don't know if you want to expand the scope of this particular patch to
include that, though.

--
Mark Dilger

Re: global / super barriers (for checksums)

[Robert Haas]

On Wed, Nov 13, 2019 at 2:45 PM Andres Freund  wrote:
> 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 haven't looked through all of these, but in AutoVacLauncherMain, a
trivial conversion to CFI doesn't seem to break anything horribly (see
attached). It does change the error message when we exit, making it
chattier, but I think we could find our way around that problem. Note
that AutoVacLauncherMain, and some of the others, do a
HOLD_INTERRUPTS() and a RESUME_INTERRUPTS() in the error-recovery
block, so somebody evidently thought some of that code might call
CHECK_FOR_INTERRUPTS(), and I can't prove off-hand that none of the
other logic which isn't so protected doesn't have some way to reach
CHECK_FOR_INTERRUPTS(). It seems to me that there are so many places
where PostgreSQL calls CHECK_FOR_INTERRUPTS() that it's somewhat
unwise to assume that just "can't happen."

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


trivial-av-conversion-cfi.patch
Description: Binary data

Re: ssl passphrase callback

[Alvaro Herrera]

On 2019-Nov-14, Andrew Dunstan wrote:

> I guess this would work. There would have to be a deal of code to load
> the library and lookup the symbol. Do we really think it's worth it?
> Leveraging shared_preload_libraries makes this comparatively simple.

Using the generic interface has the drawback that the user can make more
mistakes.  I think that's part of Bruce's issue with it (although I may
misinterpret.)

I think if you add most of it as a new entry point in dfmgr.c (where you
can leverage internal_library_load) and returns a function pointer to
the user specified function, it's all that much additional code.

(I don't think you can use load_external_function as is, because it
assumes fmgr V1 calling convention, which I'm not sure serves your case.
But then maybe it does.  And if not, then those 10 lines should be very
similar to the code you'd need to add.)

> A simpler way to handle it might be simply to error out and refuse to
> start if both ssl_passphrase_function is set and ssl_passphrase_command
> is set.

Yeah, you can do that too I guess, but I'm not sure I see that as simpler.

> Also, calling this 'ssl_passphrase_command' seems a little odd.

We could just rename ssl_passphrase_command to something more
generic, and add the existing name to map_old_guc_names to preserve
compatibility with pg12.  Maybe the new name could be simply
ssl_passphrase or perhaps ssl_passphrase_{reader,getter,pinentry}.

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

Re: SKIP_LOCKED test causes random buildfarm failures

[Tom Lane]

Andres Freund  writes:
> On 2019-11-14 13:37:44 -0500, Tom Lane wrote:
>> As for the SKIP_LOCKED tests in vacuum.sql, what I now propose is that
>> we just remove them.  I do not see that they're offering any coverage
>> that's not completely redundant with this isolation test.  We don't
>> need to spend cycles every day on that.

> -0 on that, I'd rather just put a autovacuum_enabled = false for
> them. They're quick enough, and it's nice to have decent coverage of
> various options within the plain regression tests when possible.

If we're going to keep them in vacuum.sql, we should use the
client_min_messages fix there, as that's a full solution not just
reducing the window.  But I don't agree that these tests are worth
the cycles, given the coverage elsewhere.  The probability of breaking
this option is just not high enough to justify core-regression-test
coverage.

regards, tom lane

Re: SKIP_LOCKED test causes random buildfarm failures

[Andres Freund]

Hi,

On 2019-11-14 13:37:44 -0500, Tom Lane wrote:
> I wrote:
> > Michael Paquier  writes:
> >> Good question.  That's a historical choice, still I have seen cases
> >> where those warnings are helpful while not making the logs too
> >> verbose to see some congestion in the jobs.
> 
> > I kind of feel that NOTICE is more semantically appropriate, but
> > perhaps there's an argument for keeping it at WARNING.
> 
> Well, I'm not hearing any groundswell of support for changing the
> message level, so let's leave that as-is and just see about
> stabilizing the tests.

Ok.


> >> The main purpose of the tests in regress/ is to check after the
> >> grammar, so using client_min_messages sounds like a plan.  We have
> >> a second set of tests in isolation/ where I would actually like to
> >> disable autovacuum by default on a subset of tables.  Thoughts about
> >> the attached?
> 
> After looking more closely at the isolation test, I agree that adding
> the "ALTER TABLE ... SET (autovacuum_enabled = false)" bits to it is
> a good idea.  The LOCK operations should make that irrelevant for
> part1, but there's at least a theoretical hazard for part2.
> (Actually, is "autovacuum_enabled = false" really sufficient to
> keep autovacuum away?  It'd probably lock the table for long enough
> to examine its reloptions, so it seems like all we're doing here is
> reducing the window for trouble a little bit.  Still, maybe that's
> worthwhile.)

+1


> As for the SKIP_LOCKED tests in vacuum.sql, what I now propose is that
> we just remove them.  I do not see that they're offering any coverage
> that's not completely redundant with this isolation test.  We don't
> need to spend cycles every day on that.

-0 on that, I'd rather just put a autovacuum_enabled = false for
them. They're quick enough, and it's nice to have decent coverage of
various options within the plain regression tests when possible.

Greetings,

Andres Freund

Re: Using multiple extended statistics for estimates

[Tom Lane]

Tomas Vondra  writes:
> For the case with equal Const values that should be mostly obvious, i.e.
> "a=1 AND a=1 AND a=1" has the same selectivity as "a=1".

> The case with different Const values is harder, unfortunately. It might
> seem obvious that "a=1 AND a=2" means there are no matching rows, but
> that heavily relies on the semantics of the equality operator. And we
> can't simply compare the Const values either, I'm afraid, because there
> are cases with cross-type operators like
>   a = 1::int AND a = 1.0::numeric
> where the Consts are of different type, yet both conditions can be true.

FWIW, there's code in predtest.c to handle exactly that, at least for
types sharing a btree opfamily.  Whether it's worth applying that logic
here is unclear, but note that we've had the ability to recognize
redundant and contradictory clauses for a long time:

regression=# explain select * from tenk1 where two = 1;  
 QUERY PLAN 

 Seq Scan on tenk1  (cost=0.00..470.00 rows=5000 width=244)
   Filter: (two = 1)
(2 rows)

regression=# explain select * from tenk1 where two = 1 and two = 1::bigint; 
 QUERY PLAN 

 Seq Scan on tenk1  (cost=0.00..470.00 rows=5000 width=244)
   Filter: (two = 1)
(2 rows)

regression=# explain select * from tenk1 where two = 1 and two = 2::bigint;
  QUERY PLAN   
---
 Result  (cost=0.00..470.00 rows=1 width=244)
   One-Time Filter: false
   ->  Seq Scan on tenk1  (cost=0.00..470.00 rows=1 width=244)
 Filter: (two = 1)
(4 rows)

It falls down on

regression=# explain select * from tenk1 where two = 1 and two = 2::numeric;
QUERY PLAN 
---
 Seq Scan on tenk1  (cost=0.00..520.00 rows=25 width=244)
   Filter: ((two = 1) AND ((two)::numeric = '2'::numeric))
(2 rows)

because numeric isn't in the same opfamily, so these clauses can't be
compared easily.

regards, tom lane

Re: Using multiple extended statistics for estimates

[Tomas Vondra]

On Thu, Nov 14, 2019 at 10:23:44AM -0800, Mark Dilger wrote:



On 11/14/19 7:55 AM, Tomas Vondra wrote:

On Wed, Nov 13, 2019 at 10:04:36AM -0800, Mark Dilger wrote:



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.




Thanks.

1) It's not clear to me why adding 'const' to the List parameters would
  be useful? Can you explain?


When I first started reviewing the functions, I didn't know if those 
lists were intended to be modified by the function.  Adding 'const' 
helps document that the function does not intend to change them.




Hmmm, ok. I'll think about it, but we're not really using const* in this
way very much I think - at least not in the surrounding code.


2) I think you're right we can change find_strongest_dependency to do

   /* also skip weaker dependencies when attribute count matches */
   if (strongest->nattributes == dependency->nattributes &&
   strongest->degree >= dependency->degree)
   continue;

  That'll skip some additional dependencies, which seems OK.

3) It's not clear to me what you mean by

    * 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.

  added to dependencies_clauselist_selectivity. Are you saying we
  should also compute selectivity estimates for individual clauses and
  use Min() as a limit? Maybe, but that seems unrelated to the patch.


I mean that the comment right above that TODO is hard to understand. 
You seem to be saying that it is good and proper to only take the 
selectivity estimate from the final clause in the list, but then go on 
to say that other clauses might prove that no rows will match.  So 
that implies that by ignoring all but the last clause, we're ignoring 
such other clauses that prove no rows can match.  But why would we be 
ignoring those?


I am not arguing that your code is wrong.  I'm just critiquing the 
hard-to-understand phrasing of that code comment.




Aha, I think I understand now - thanks for the explanation. You're right
the comment is trying to explain why just taking the last clause for a
given attnum is fine. I'll try to make the comment clearer.

For the case with equal Const values that should be mostly obvious, i.e.
"a=1 AND a=1 AND a=1" has the same selectivity as "a=1".

The case with different Const values is harder, unfortunately. It might
seem obvious that "a=1 AND a=2" means there are no matching rows, but
that heavily relies on the semantics of the equality operator. And we
can't simply compare the Const values either, I'm afraid, because there
are cases with cross-type operators like

 a = 1::int AND a = 1.0::numeric

where the Consts are of different type, yet both conditions can be true.

So it would be pretty tricky to do this, and the current code does not
even try to do that.

Instead, it just assumes that it's mostly fine to overestimate, because
then at runtime we'll simply end up with 0 rows here.


regards

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

Re: format of pg_upgrade loadable_libraries warning

[Tom Lane]

Alvaro Herrera  writes:
> On 2019-Oct-07, Bruce Momjian wrote:
>> and the "in database" (which I have changed to capitalized "In database"
>> in the attached patch), looks like:
>> fprintf(script, "In database: %s\n", active_db->db_name);
>> meaning it _isn't_ an output error message, but rather something that
>> appears in an error file.  I don't think either of these are translated.
>> Is that wrong?

> pg_fatal is a "gettext trigger" (see nls.mk), so that part of the
> message is definitely translated.

Right, but Bruce's point is that what goes into the separate output
file listing problem cases is not translated, and never has been.
Maybe we should start doing so, but that would be a distinct issue.
I'm not really sure that we should translate it, anyway --- could
there be anyone out there who is using tools to process these files?

> BTW, how is one supposed to "manually upgrade databases that use
> contrib/isb"?  This part is not very clear.

Agreed, the pg_fatal message is claiming that you can do something
without really providing any concrete instructions for it.  I'm not
sure that that's helpful.

regards, tom lane

Re: ssl passphrase callback

[Andrew Dunstan]

On 11/14/19 12:07 PM, Alvaro Herrera wrote:
> On 2019-Nov-14, Bruce Momjian wrote:
>
>> I was assuming if the variable starts with a #, it is a shared object,
>> if not, it is a shell command:
>>
>>  ssl_passphrase_command='#/lib/x.so'
>>  ssl_passphrase_command='my_command a b c'
> Note that the proposed patch doesn't use a separate GUC -- it just uses
> shared_preload_libraries, and then it is the library that's in charge of
> setting up the function.  We probably wouldn't like to have multiple
> settings that all do the same thing, such as recovery target (which
> seems to be a plentiful source of confusion).
>
> Changing the interface so that the user has to specify the function name
> (not the library name) in ssl_passphrase_command closes that ambiguity
> hole.
>
> Note that if you specify only the library name, it becomes redundant
> w.r.t. shared_preload_libraries; you could have more than one library
> setting the function callback and it's hard to see which one wins.
>
> I think something like this would do it:
>   ssl_passphrase_command='#superlib.so,my_rot13_passphrase'
>
> This way, the library can still create any custom GUCs it pleases/needs,
> but there's no possible confusion as to the function that's going to be
> called.


I guess this would work. There would have to be a deal of code to load
the library and lookup the symbol. Do we really think it's worth it?
Leveraging shared_preload_libraries makes this comparatively simple.


Also, calling this 'ssl_passphrase_command' seems a little odd.


A simpler way to handle it might be simply to error out and refuse to
start if both ssl_passphrase_function is set and ssl_passphrase_command
is set.


cheers


andrew

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

Re: Option to dump foreign data in pg_dump

[Tom Lane]

Alvaro Herrera  writes:
> On 2019-Nov-12, Luis Carril wrote:
>> But, not all foreign tables are necessarily in a remote server like
>> the ones referenced by the postgres_fdw.
>> In FDWs like swarm64da, cstore, citus or timescaledb, the foreign
>> tables are part of your database, and one could expect that a dump of
>> the database includes data from these FDWs.

> BTW these are not FDWs in the "foreign" sense at all; they're just
> abusing the FDW system in order to be able to store data in some
> different way.  The right thing to do IMO is to port these systems to be
> users of the new storage abstraction (table AM).  If we do that, what
> value is there to the feature being proposed here?

That is a pretty valid point.  I'm not sure however that there would
be *no* use-cases for the proposed option if all of those FDWs were
converted to table AMs.  Also, even if the authors of those systems
are all hard at work on such a conversion, it'd probably be years
before the FDW implementations disappear from the wild.

Having said that, I'm ending up -0.5 or so on the patch as it stands,
mainly because it seems like it is bringing way more maintenance
burden than it's realistically worth.  I'm particularly unhappy about
the proposed regression test additions --- the cycles added to
check-world, and the maintenance effort that's inevitably going to be
needed for all that code, seem unwarranted for something that's at
best a very niche use-case.  And, despite the bulk of the test
additions, they're in no sense offering an end-to-end test, because
that would require successfully reloading the data as well.

That objection could be addressed, perhaps, by scaling down the tests
to just have a goal of exercising the new pg_dump option-handling
code, and not to attempt to do meaningful data extraction from a
foreign table.  You could do that with an entirely dummy foreign data
wrapper and server (cf. sql/foreign_data.sql).  I'm imagining perhaps
create two dummy servers, of which only one has a table, and we ask to
dump data from the other one.  This would cover parsing and validation
of the --include-foreign-data option, and make sure that we don't dump
from servers we're not supposed to.  It doesn't actually dump any
data, but that part is a completely trivial aspect of the patch,
really, and almost all of the code relevant to that does get tested
already.

In the department of minor nitpicks ... why bother with joining to
pg_foreign_server in the query that retrieves a foreign table's
server OID?  ft.ftserver is already the answer you seek.  Also,
I think it'd be wise from a performance standpoint to skip doing
that query altogether in the normal case where --include-foreign-data
hasn't been requested.

regards, tom lane

Re: format of pg_upgrade loadable_libraries warning

[Alvaro Herrera]

On 2019-Oct-07, Bruce Momjian wrote:

> Uh, I looked at the pg_ugprade code and the error message is:
> 
> pg_fatal("Your installation contains \"contrib/isn\" functions which 
> rely on the\n"
>  "bigint data type.  Your old and new clusters pass bigint 
> values\n"
>  "differently so this cluster cannot currently be upgraded.  
> You can\n"
>  "manually upgrade databases that use \"contrib/isn\" 
> facilities and remove\n"
>  "\"contrib/isn\" from the old cluster and restart the 
> upgrade.  A list of\n"
>  "the problem functions is in the file:\n"
>  "%s\n\n", output_path);
> 
> and the "in database" (which I have changed to capitalized "In database"
> in the attached patch), looks like:
> 
>fprintf(script, "In database: %s\n", active_db->db_name);
> 
> meaning it _isn't_ an output error message, but rather something that
> appears in an error file.  I don't think either of these are translated.
> Is that wrong?

pg_fatal is a "gettext trigger" (see nls.mk), so that part of the
message is definitely translated.  And the fprintf format string should
be decorated with _() in order to make translatable too; otherwise the
message is only half-translated when it appears in the pg_upgrade log,
which is not nice.  This should look like:

if (!db_used)
{
/* translator: This is an error message 
indicator */
fprintf(script, _("In database: %s\n"), 
active_db->db_name);
db_used = true;
}
fprintf(script, "  %s.%s\n",


BTW, how is one supposed to "manually upgrade databases that use
contrib/isb"?  This part is not very clear.

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

Re: SKIP_LOCKED test causes random buildfarm failures

[Tom Lane]

I wrote:
> Michael Paquier  writes:
>> Good question.  That's a historical choice, still I have seen cases
>> where those warnings are helpful while not making the logs too
>> verbose to see some congestion in the jobs.

> I kind of feel that NOTICE is more semantically appropriate, but
> perhaps there's an argument for keeping it at WARNING.

Well, I'm not hearing any groundswell of support for changing the
message level, so let's leave that as-is and just see about
stabilizing the tests.

>> The main purpose of the tests in regress/ is to check after the
>> grammar, so using client_min_messages sounds like a plan.  We have
>> a second set of tests in isolation/ where I would actually like to
>> disable autovacuum by default on a subset of tables.  Thoughts about
>> the attached?

After looking more closely at the isolation test, I agree that adding
the "ALTER TABLE ... SET (autovacuum_enabled = false)" bits to it is
a good idea.  The LOCK operations should make that irrelevant for
part1, but there's at least a theoretical hazard for part2.
(Actually, is "autovacuum_enabled = false" really sufficient to
keep autovacuum away?  It'd probably lock the table for long enough
to examine its reloptions, so it seems like all we're doing here is
reducing the window for trouble a little bit.  Still, maybe that's
worthwhile.)

As for the SKIP_LOCKED tests in vacuum.sql, what I now propose is that
we just remove them.  I do not see that they're offering any coverage
that's not completely redundant with this isolation test.  We don't
need to spend cycles every day on that.

regards, tom lane

Re: Using multiple extended statistics for estimates

[Mark Dilger]

On 11/14/19 7:55 AM, Tomas Vondra wrote:

On Wed, Nov 13, 2019 at 10:04:36AM -0800, Mark Dilger wrote:



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.




Thanks.

1) It's not clear to me why adding 'const' to the List parameters would
   be useful? Can you explain?


When I first started reviewing the functions, I didn't know if those 
lists were intended to be modified by the function.  Adding 'const' 
helps document that the function does not intend to change them.



2) I think you're right we can change find_strongest_dependency to do

    /* also skip weaker dependencies when attribute count matches */
    if (strongest->nattributes == dependency->nattributes &&
    strongest->degree >= dependency->degree)
    continue;

   That'll skip some additional dependencies, which seems OK.

3) It's not clear to me what you mean by

     * 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.

   added to dependencies_clauselist_selectivity. Are you saying we
   should also compute selectivity estimates for individual clauses and
   use Min() as a limit? Maybe, but that seems unrelated to the patch.


I mean that the comment right above that TODO is hard to understand. You 
seem to be saying that it is good and proper to only take the 
selectivity estimate from the final clause in the list, but then go on 
to say that other clauses might prove that no rows will match.  So that 
implies that by ignoring all but the last clause, we're ignoring such 
other clauses that prove no rows can match.  But why would we be 
ignoring those?


I am not arguing that your code is wrong.  I'm just critiquing the 
hard-to-understand phrasing of that code comment.


--
Mark Dilger

Re: Proposal: Add more compile-time asserts to expose inconsistencies.

[Andres Freund]

Hi,

On 2019-11-13 03:23:06 +, Smith, Peter wrote:
> From: Andres Freund  Sent: Wednesday, 13 November 2019 
> 6:01 AM
> 
> >Peter Smith:
> >
> > Is there a reason to not just make StaticAssertExpr and StaticAssertStmt be 
> > the same? I don't want to proliferate variants that users have to 
> > understand if there's no compelling 
> > need.  Nor do I think do we really need two different fallback 
> > implementation for static asserts.
> 
> >
> > As far as I can tell we should be able to use the prototype based approach 
> > in all the cases where we currently use the "negative bit-field width" 
> > approach?
> 
> I also thought that the new "prototype negative array-dimension" based
> approach (i.e. StaticAssertDecl) looked like an improvement over the
> existing "negative bit-field width" approach (i.e. StaticAssertStmt),
> because it seems to work for more scenarios (e.g. file scope).
> 
> But I did not refactor existing code to use the new way because I was
> fearful that there might be some subtle reason why the
> StaticAssertStmt was deliberately made that way (e.g. as do/while),
> and last thing I want to do was break working code.

That'll just leave us with cruft. And it's not like this stuff will
break in a subtle way or such

Greetings,

Andres Freund

Re: ssl passphrase callback

[Bruce Momjian]

On Thu, Nov 14, 2019 at 02:07:58PM -0300, Alvaro Herrera wrote:
> On 2019-Nov-14, Bruce Momjian wrote:
> 
> > I was assuming if the variable starts with a #, it is a shared object,
> > if not, it is a shell command:
> > 
> > ssl_passphrase_command='#/lib/x.so'
> > ssl_passphrase_command='my_command a b c'
> 
> Note that the proposed patch doesn't use a separate GUC -- it just uses
> shared_preload_libraries, and then it is the library that's in charge of
> setting up the function.  We probably wouldn't like to have multiple
> settings that all do the same thing, such as recovery target (which
> seems to be a plentiful source of confusion).
> 
> Changing the interface so that the user has to specify the function name
> (not the library name) in ssl_passphrase_command closes that ambiguity
> hole.
> 
> Note that if you specify only the library name, it becomes redundant
> w.r.t. shared_preload_libraries; you could have more than one library
> setting the function callback and it's hard to see which one wins.
> 
> I think something like this would do it:
>   ssl_passphrase_command='#superlib.so,my_rot13_passphrase'
> 
> This way, the library can still create any custom GUCs it pleases/needs,
> but there's no possible confusion as to the function that's going to be
> called.

Yeah, I was unclear how the function name would be specified.  I thought
it would just be hard-coded, but I like the above better.  I am still
unclear how parameters are passed.

-- 
  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: ssl passphrase callback

[Alvaro Herrera]

On 2019-Nov-14, Bruce Momjian wrote:

> I was assuming if the variable starts with a #, it is a shared object,
> if not, it is a shell command:
> 
>   ssl_passphrase_command='#/lib/x.so'
>   ssl_passphrase_command='my_command a b c'

Note that the proposed patch doesn't use a separate GUC -- it just uses
shared_preload_libraries, and then it is the library that's in charge of
setting up the function.  We probably wouldn't like to have multiple
settings that all do the same thing, such as recovery target (which
seems to be a plentiful source of confusion).

Changing the interface so that the user has to specify the function name
(not the library name) in ssl_passphrase_command closes that ambiguity
hole.

Note that if you specify only the library name, it becomes redundant
w.r.t. shared_preload_libraries; you could have more than one library
setting the function callback and it's hard to see which one wins.

I think something like this would do it:
  ssl_passphrase_command='#superlib.so,my_rot13_passphrase'

This way, the library can still create any custom GUCs it pleases/needs,
but there's no possible confusion as to the function that's going to be
called.

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

Re: ssl passphrase callback

[Bruce Momjian]

On Thu, Nov 14, 2019 at 11:34:24AM -0500, Andrew Dunstan wrote:
> 
> On 11/14/19 11:07 AM, Bruce Momjian wrote:
> > On Thu, Nov 14, 2019 at 11:42:05AM +0100, Magnus Hagander wrote:
> >> On Wed, Nov 13, 2019 at 9:23 PM Tomas Vondra 
> >> 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.
> >>
> >>
> >> Yeah, that probably wouldn't hurt. It's also securely passing from more 
> >> than
> >> one perspective -- both from the "cannot be eavesdropped" (like putting the
> >> password on the commandline for example) and the requirement for escaping.
> > I think a bigger issue is that if you want to give people the option of
> > using a shell command or a shared object, and if you use two commands to
> > control it, it isn't clear what happens if both are defined.  By using
> > some character prefix to control if a shared object is used, you can use
> > a single variable and there is no confusion over having two variables
> > and their conflicting behavior.
> >
> 
> 
> I'm  not sure how that would work in the present instance. The shared
> preloaded module installs a function and defines the params it wants. If
> we somehow unify the params with ssl_passphrase_command that could look
> icky, and the module would have to parse the settings string. That's not
> a problem for the sample module which only needs one param, but it will
> be for other more complex implementations.
> 
> I'm quite open to suggestions, but I want things to be tolerably clean.

I was assuming if the variable starts with a #, it is a shared object,
if not, it is a shell command:

ssl_passphrase_command='#/lib/x.so'
ssl_passphrase_command='my_command a b c'

Can you show what you are talking about?

-- 
  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: ssl passphrase callback

[Tomas Vondra]

On Thu, Nov 14, 2019 at 11:34:24AM -0500, Andrew Dunstan wrote:


On 11/14/19 11:07 AM, Bruce Momjian wrote:

On Thu, Nov 14, 2019 at 11:42:05AM +0100, Magnus Hagander wrote:

On Wed, Nov 13, 2019 at 9:23 PM Tomas Vondra 
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.


Yeah, that probably wouldn't hurt. It's also securely passing from more than
one perspective -- both from the "cannot be eavesdropped" (like putting the
password on the commandline for example) and the requirement for escaping.

I think a bigger issue is that if you want to give people the option of
using a shell command or a shared object, and if you use two commands to
control it, it isn't clear what happens if both are defined.  By using
some character prefix to control if a shared object is used, you can use
a single variable and there is no confusion over having two variables
and their conflicting behavior.




I'm  not sure how that would work in the present instance. The shared
preloaded module installs a function and defines the params it wants. If
we somehow unify the params with ssl_passphrase_command that could look
icky, and the module would have to parse the settings string. That's not
a problem for the sample module which only needs one param, but it will
be for other more complex implementations.

I'm quite open to suggestions, but I want things to be tolerably clean.



I agree it's better to have two separate GUCs - one for command, one for
shared object, and documented order of precedence. I suppose we may log
a warning when both are specified, or perhaps "reset" the value with
lower order of precedence.

regards

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

Re: ssl passphrase callback

[Andrew Dunstan]

On 11/14/19 11:07 AM, Bruce Momjian wrote:
> On Thu, Nov 14, 2019 at 11:42:05AM +0100, Magnus Hagander wrote:
>> On Wed, Nov 13, 2019 at 9:23 PM Tomas Vondra 
>> 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.
>>
>>
>> Yeah, that probably wouldn't hurt. It's also securely passing from more than
>> one perspective -- both from the "cannot be eavesdropped" (like putting the
>> password on the commandline for example) and the requirement for escaping.
> I think a bigger issue is that if you want to give people the option of
> using a shell command or a shared object, and if you use two commands to
> control it, it isn't clear what happens if both are defined.  By using
> some character prefix to control if a shared object is used, you can use
> a single variable and there is no confusion over having two variables
> and their conflicting behavior.
>


I'm  not sure how that would work in the present instance. The shared
preloaded module installs a function and defines the params it wants. If
we somehow unify the params with ssl_passphrase_command that could look
icky, and the module would have to parse the settings string. That's not
a problem for the sample module which only needs one param, but it will
be for other more complex implementations.

I'm quite open to suggestions, but I want things to be tolerably clean.


cheers


andrew



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

Re: ssl passphrase callback

[Bruce Momjian]

On Thu, Nov 14, 2019 at 11:42:05AM +0100, Magnus Hagander wrote:
> On Wed, Nov 13, 2019 at 9:23 PM Tomas Vondra 
> 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.
> 
> 
> Yeah, that probably wouldn't hurt. It's also securely passing from more than
> one perspective -- both from the "cannot be eavesdropped" (like putting the
> password on the commandline for example) and the requirement for escaping.

I think a bigger issue is that if you want to give people the option of
using a shell command or a shared object, and if you use two commands to
control it, it isn't clear what happens if both are defined.  By using
some character prefix to control if a shared object is used, you can use
a single variable and there is no confusion over having two variables
and their conflicting behavior.

-- 
  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: Using multiple extended statistics for estimates

[Tomas Vondra]

On Wed, Nov 13, 2019 at 10:04:36AM -0800, Mark Dilger wrote:



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.




Thanks.

1) It's not clear to me why adding 'const' to the List parameters would
  be useful? Can you explain?

2) I think you're right we can change find_strongest_dependency to do

   /* also skip weaker dependencies when attribute count matches */
   if (strongest->nattributes == dependency->nattributes &&
   strongest->degree >= dependency->degree)
   continue;

  That'll skip some additional dependencies, which seems OK.

3) It's not clear to me what you mean by

* 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.

  added to dependencies_clauselist_selectivity. Are you saying we
  should also compute selectivity estimates for individual clauses and
  use Min() as a limit? Maybe, but that seems unrelated to the patch.

regards

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

Re: segfault in geqo on experimental gcc animal

[Martin Liška]

Hi.

Yep, I build periodically PostgreSQL package in openSUSE with the
latest GCC and so
that I identified that and isolated to a simple test-case. I would expect a fix
today or tomorrow.

See you,
Martin

On Thu, 14 Nov 2019 at 16:46, Fabien COELHO  wrote:
>
>
> Hello,
>
> I did a (slow) dichotomy on gcc sources which determined that gcc r277979
> was the culprit, then I started a bug report which showed that the issue
> was already reported this morning by Martin Liška, including a nice
> example isolated from sources. See:
>
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92506
>
> --
> Fabien.

Re: could not stat promote trigger file leads to shutdown

[Tom Lane]

Peter Eisentraut  writes:
> I have seen the error
>  could not stat promote trigger file "...": Permission denied
> because of a misconfiguration (for example, setting promote_trigger_file 
> to point into a directory to which you don't have appropriate read or 
> execute access).

> The problem is that because this happens in the startup process, the 
> ERROR is turned into a FATAL and the whole instance shuts down.  That 
> seems like a harsh penalty.  Would it be better to turn this ERROR into 
> a WARNING?

It is harsh, but I suspect if we just logged the complaint, we'd get
bug reports about "Postgres isn't reacting to my trigger file",
because people don't read the postmaster log unless forced to.
Is there some more-visible way to report the problem, short of
shutting down?

(BTW, from this perspective, WARNING is especially bad because it
might not get logged at all.  Better to use LOG.)

One thought is to try to detect the misconfiguration at postmaster
start --- better to fail at startup than sometime later.  But I'm
not sure how reliably we could do that.

regards, tom lane

Re: 2019-11-14 Press Release Draft

[Jonathan S. Katz]

On 11/14/19 7:46 AM, Geoff Winkless wrote:
> On Tue, 12 Nov 2019 at 22:17, Jonathan S. Katz  wrote:
>> Attached is a draft of the press release for the update release going
>> out on 2010-11-14. Please provide feedback, particularly on the
>> technical accuracy of the statements.
> 
> Text
> 
> by the `position()`
> 
> should probably either be
> 
>by `position()`
> 
> or
> 
> by the `position()` function

Thanks Geoff & Sehrope for your suggestions / corrections. I have
incorporated them, as well as a few other things I noticed as well.

The release is now out!

Jonathan



signature.asc
Description: OpenPGP digital signature

Re: Invisible PROMPT2

[Tom Lane]

Kyotaro Horiguchi  writes:
> 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.

Oh, that's a good point.  But it actually leads to a much simpler
definition and implementation than the other ideas we've kicked
around: define %w as "whitespace equal to the length of the
last-generated PROMPT1 string (initially empty)", and we just
have to save PROMPT1 each time we generate it.

Except ... I'm not sure how to deal with hidden escape sequences.
We should probably assume that anything inside %[...%] has width
zero, but how would we remember that?

Maybe count the width of non-escape characters whenever we
generate PROMPT1, and just save that number not the string?
It'd add overhead that's useless when there's no %w, but
probably not enough to care about.

regards, tom lane

Re: Proposal: Add more compile-time asserts to expose inconsistencies.

[Peter Eisentraut]

On 2019-11-12 20:00, Andres Freund wrote:

Looking at the cplusplus variant, I'm somewhat surprised to see that you
made both fallback and plain version unconditionally use GCC style
compound expressions:



Was that intentional? The C version intentionally uses compound
expressions only for the _Static_assert case, where configure tests for
the compound expression support?  As far as I can tell this'll not allow
using our headers e.g. with msvc in C++ mode if somebody introduce a
static assertion in a header - which seems like a likely and good
outcome with the changes proposed here?


I don't recall all the details anymore, but if you're asking, why is the 
fallback implementation in C++ different from the one in C, then that's 
because the C variant didn't work in C++.


I seem to recall that I did this work in order to get an actual 
C++-using extension to compile, so it worked(tm) at some point, but I 
probably didn't try it with a not-gcc compatible compiler at the time.


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

Re: segfault in geqo on experimental gcc animal

[Fabien COELHO]

Hello,

I did a (slow) dichotomy on gcc sources which determined that gcc r277979 
was the culprit, then I started a bug report which showed that the issue 
was already reported this morning by Martin Liška, including a nice 
example isolated from sources. See:


https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92506

--
Fabien.

Re: MarkBufferDirtyHint() and LSN update

[Antonin Houska]

Michael Paquier  wrote:

> 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.

Interesting. Now that I know about the problem, I could have reproduced it
using gdb: MarkBufferDirtyHint() was called by 2 backends concurrently in such
a way that the first backend generates the LSN, but before it manages to
assign it to the page, another backend generates another LSN and sets it.

Can't we just apply the attached diff on the top of your patch?

Also I wonder how checksums helped you to discover the problem? Although I
could simulate the setting of lower LSN, I could not see any broken
checksum. And I wouldn't even expect that since FlushBuffer() copies the
buffer into backend local memory before it calculates the checksum.

-- 
Antonin Houska
Web: https://www.cybertec-postgresql.com

diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index eba9a4716d..2bbc5a92c7 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -3540,11 +3540,14 @@ MarkBufferDirtyHint(Buffer buffer, bool buffer_std)
 		 * before using PageGetLSN(), which is enforced in
 		 * BufferGetLSNAtomic().
 		 *
+		 * Do not let LSN go backwards. This might happen if a concurrent
+		 * backend generated the LSN later that we did but applied before us.
+		 *
 		 * If checksums are enabled, you might think we should reset the
 		 * checksum here. That will happen when the page is written sometime
 		 * later in this checkpoint cycle.
 		 */
-		if (!XLogRecPtrIsInvalid(lsn))
+		if (!XLogRecPtrIsInvalid(lsn) && lsn > PageGetLSN(page))
 			PageSetLSN(page, lsn);
 
 		buf_state |= BM_DIRTY | BM_JUST_DIRTIED;

Re: could not stat promote trigger file leads to shutdown

[Fujii Masao]

On Thu, Nov 14, 2019 at 10:58 PM Peter Eisentraut
 wrote:
>
> I have seen the error
>
>  could not stat promote trigger file "...": Permission denied
>
> because of a misconfiguration (for example, setting promote_trigger_file
> to point into a directory to which you don't have appropriate read or
> execute access).
>
> The problem is that because this happens in the startup process, the
> ERROR is turned into a FATAL and the whole instance shuts down.  That
> seems like a harsh penalty.  Would it be better to turn this ERROR into
> a WARNING?

+1

Regards,

-- 
Fujii Masao

Re: Allow CREATE OR REPLACE VIEW to rename the columns

[Fujii Masao]

On Wed, Nov 6, 2019 at 4:14 PM btfujiitkp  wrote:
>
> 2019-10-31 21:01 に Fujii Masao さんは書きました:
> > On Thu, Oct 31, 2019 at 7:59 PM Ibrar Ahmed 
> > wrote:
> >>
> >>
> >>
> >> On Thu, Oct 31, 2019 at 12:32 PM Fujii Masao 
> >> wrote:
> >>>
> >>> On Thu, Oct 31, 2019 at 1:42 PM Tom Lane  wrote:
> >>> >
> >>> > Fujii Masao  writes:
> >>> > > Currently CREATE OR REPLACE VIEW command fails if the column names
> >>> > > are changed.
> >>> >
> >>> > That is, I believe, intentional.  It's an effective aid to catching
> >>> > mistakes in view redefinitions, such as misaligning the new set of
> >>> > columns relative to the old.  That's particularly important given
> >>> > that we allow you to add columns during CREATE OR REPLACE VIEW.
> >>> > Consider the oversimplified case where you start with
> >>> >
> >>> > CREATE VIEW v AS SELECT 1 AS x, 2 AS y;
> >>> >
> >>> > and you want to add a column z, and you get sloppy and write
> >>> >
> >>> > CREATE OR REPLACE VIEW v AS SELECT 1 AS x, 3 AS z, 2 AS y;
> >>> >
> >>> > If we did not throw an error on this, references that formerly
> >>> > pointed to column y would now point to z (as that's still attnum 2),
> >>> > which is highly unlikely to be what you wanted.
> >>>
> >>> This example makes me wonder if the addtion of column by
> >>> CREATE OR REPLACE VIEW also has the same (or even worse) issue.
> >>> That is, it may increase the oppotunity for users' mistake.
> >>> I'm thinking the case where users mistakenly added new column
> >>> into the view when replacing the view definition. This mistake can
> >>> happen because CREATE OR REPLACE VIEW allows new column to
> >>> be added. But what's the worse is that, currently there is no way to
> >>> drop the column from the view, except recreation of the view.
> >>> Neither CREATE OR REPLACE VIEW nor ALTER TABLE support
> >>> the drop of the column from the view. So, to fix the mistake,
> >>> users would need to drop the view itself and recreate it. If there
> >>> are
> >>> some objects depending the view, they also might need to be
> >>> recreated.
> >>> This looks not good. Since the feature has been supported,
> >>> it's too late to say that, though...
> >>>
> >>> At least, the support for ALTER VIEW DROP COLUMN might be
> >>> necessary to alleviate that situation.
> >>>
> >>
> >> - Is this intentional not implemented the "RENAME COLUMN" statement
> >> for
> >> VIEW because it is implemented for Materialized View?
> >
> > Not sure that, but Tom's suggestion to support ALTER VIEW RENAME COLUMN
> > sounds reasonable whether we support the rename of columns when
> > replacing
> > the view definition, or not. Attached is the patch that adds support
> > for
> > ALTER VIEW RENAME COLUMN command.
> >
> >> I have made just a similar
> >> change to view and it works.
> >
> > Yeah, ISTM that we made the same patch at the same time. You changed
> > gram.y,
> > but I added the following changes additionally.
> >
> > - Update the doc
> > - Add HINT message emit when CRAETE OR REPLACE VIEW fails to rename the
> > columns
> > - Update tab-complete.c
>
>
> I review your patch, and then I found that tab complete of "alter
> materialized view" is also not enough.
> So, I made a small patch referencing your patch.

Good catch! The patch basically looks good to me.
But I think that "ALTER MATERIALIZED VIEW xxx " should output also
DEPENDS ON EXTENSION, SET TABLESPACE, CLUSTER ON and RESET.
So I added such tab-completes to your patch. Patch attached.

Barring any objection, I'm thinking to commit this patch.

Regards,

-- 
Fujii Masao


alter_materialized_view_v2.patch
Description: Binary data

could not stat promote trigger file leads to shutdown

[Peter Eisentraut]

I have seen the error

could not stat promote trigger file "...": Permission denied

because of a misconfiguration (for example, setting promote_trigger_file 
to point into a directory to which you don't have appropriate read or 
execute access).


The problem is that because this happens in the startup process, the 
ERROR is turned into a FATAL and the whole instance shuts down.  That 
seems like a harsh penalty.  Would it be better to turn this ERROR into 
a WARNING?


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

Re: ssl passphrase callback

[Sehrope Sarkuni]

On Wed, Nov 13, 2019 at 3:23 PM Tomas Vondra
 wrote:
> 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.

External command args can be viewed by other OS users (not just the
postgres user). For non-sensitive arguments (ex: WAL filename?) that's
not an issue but if you plan on passing in something potentially
secret value from the backend to the external OS command, that value
would be exposed:

Terminal 1 (run a command as some other user):
$ sudo -u nobody sleep 5

Terminal 2 (view command args as a different non-super user):
$ ps -u nobody -o args
COMMAND
sleep 5

A shared library would not have this problem as the backend directly
executes the library in the same process.

Has the idea of using environment variables (rather than command line
args) for external commands been brought up before? I couldn't find
anything in the mailing list archives.

Environment variables have the advantage of only being readable by the
process owner and super user. They also naturally have a "name" and do
not have escaping or quoting issues.

For example, archive_command %p could be exposed as
PG_ARG_ARCHIVE_PATH and %f could be exposed as
PG_ARG_ARCHIVE_FILENAME. Then you could have a script use them via:

#!/usr/bin/env bash
set -euo pipefail
main () {
local archive_dir="/path/to/archive_dir/"
local archive_file="${archive_dir}${PG_ARG_ARCHIVE_FILENAME}"
test ! -f "${archive_file}" && cp -- "${PG_ARG_ARCHIVE_PATH}"
"${archive_file}"
}
main "$@"

It's not particularly useful for that basic archive case but if
there's something like PG_ARG_SUPER_SECRET then the executed command
could receive that value without it being exposed. That'd be useful
for something like a callout to an external KMS (key management
system).

Nothing stops something like this with being used in tandem with
string substitution to create the full commands. That'd give backwards
compatibility too. The main limitation compared to a shared library is
that you'd still have to explicitly pick and name the exposed argument
environment variables (i.e. like picking the set of %? substitutions).
If there's a generic shared-library-for-external-commands approach
then there could be a built-in library that provides this type of
"expose as env vars" functionality.

Regards,
-- Sehrope Sarkuni
Founder & CEO | JackDB, Inc. | https://www.jackdb.com/

Re: Optimize partial TOAST decompression

[Tomas Vondra]

On Thu, Nov 14, 2019 at 03:27:42PM +0530, Rushabh Lathia wrote:

Today I noticed strange behaviour, consider the following test:

postgres@126111=#create table foo ( a text );
CREATE TABLE
postgres@126111=#insert into foo values ( repeat('PostgreSQL is the
world''s best database and leading by an Open Source Community.', 8000));
INSERT 0 1

postgres@126111=#select substring(a from 639921 for 81) from foo;
substring
---

(1 row)



Hmmm. I think the issue is heap_tuple_untoast_attr_slice is using the
wrong way to determine compressed size in the VARATT_IS_EXTERNAL_ONDISK
branch. It does this

max_size = pglz_maximum_compressed_size(sliceoffset + slicelength,
TOAST_COMPRESS_SIZE(attr));

But for the example you've posted TOAST_COMPRESS_SIZE(attr) returns 10,
which is obviously bogus because the TOAST table contains ~75kB of data.

I think it should be doing this instead:

max_size = pglz_maximum_compressed_size(sliceoffset + slicelength,
toast_pointer.va_extsize);

At least that fixes it for me.

I wonder if this actually explains the crashes 540f3168091 was supposed
to fix, but it just masked them instead.

regards

--
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
diff --git a/src/backend/access/common/detoast.c 
b/src/backend/access/common/detoast.c
index 47a03fa98b..b796406239 100644
--- a/src/backend/access/common/detoast.c
+++ b/src/backend/access/common/detoast.c
@@ -233,7 +233,7 @@ heap_tuple_untoast_attr_slice(struct varlena *attr,
 * of a given length (after decompression).
 */
max_size = pglz_maximum_compressed_size(sliceoffset + 
slicelength,
-   
TOAST_COMPRESS_SIZE(attr));
+   
toast_pointer.va_extsize);
 
/*
 * Fetch enough compressed slices (compressed marker 
will get set

Re: 2019-11-14 Press Release Draft

[Geoff Winkless]

On Tue, 12 Nov 2019 at 22:17, Jonathan S. Katz  wrote:
> Attached is a draft of the press release for the update release going
> out on 2010-11-14. Please provide feedback, particularly on the
> technical accuracy of the statements.

Text

by the `position()`

should probably either be

   by `position()`

or

by the `position()` function

no?

Geoff

Re: 2019-11-14 Press Release Draft

[Sehrope Sarkuni]

> * Several fixes for logical replication, including a failure when the 
> publisher
> & subscriber had different REPLICA IDENTITY columns set.

"&" should probably be "and" as I don't see it used like that in any
other release notes.

Regards,
-- Sehrope Sarkuni
Founder & CEO | JackDB, Inc. | https://www.jackdb.com/

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

[didier]

Hi,

On Wed, Nov 13, 2019 at 10:01 PM Tom Lane  wrote:
>
> 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 :-)
Yes, that's it. I should have known better, it's no the first time I
made this mistake,
thanks.

Didier

Re: cost based vacuum (parallel)

[Mahendra Singh]

On Mon, 11 Nov 2019 at 17:56, Amit Kapila  wrote:
>
> On Mon, Nov 11, 2019 at 5:14 PM Dilip Kumar  wrote:
> >
> > On Mon, Nov 11, 2019 at 4:23 PM Amit Kapila 
wrote:
> > >
> > > ..
> > > > I have tested the same with some other workload(test file attached).
> > > > I can see the same behaviour with this workload as well that with
the
> > > > patch 4 the distribution of the delay is better compared to other
> > > > patches i.e. worker with more I/O have more delay and with equal IO
> > > > have alsomost equal delay.  Only thing is that the total delay with
> > > > the patch 4 is slightly less compared to other pacthes.
> > > >
> > >
> > > I see one problem with the formula you have used in the patch, maybe
> > > that is causing the value of total delay to go down.
> > >
> > > - if (new_balance >= VacuumCostLimit)
> > > + VacuumCostBalanceLocal += VacuumCostBalance;
> > > + if ((new_balance >= VacuumCostLimit) &&
> > > + (VacuumCostBalanceLocal > VacuumCostLimit/(0.5 * nworker)))
> > >
> > > As per discussion, the second part of the condition should be
> > > "VacuumCostBalanceLocal > (0.5) * VacuumCostLimit/nworker".
> > My Bad
> > I think
> > > you can once change this and try again.  Also, please try with the
> > > different values of threshold (0.3, 0.5, 0.7, etc.).
> > >
> > Okay, I will retest with both patch3 and path4 for both the scenarios.
> > I will also try with different multipliers.
> >
>
> One more thing, I think we should also test these cases with a varying
> number of indexes (say 2,6,8,etc.) and then probably, we should test
> by a varying number of workers where the number of workers are lesser
> than indexes.  You can do these after finishing your previous
> experiments.

On the top of parallel vacuum patch, I applied Dilip's
patch(0001-vacuum_costing_test.patch). I have tested by varying number of
indexes and number of workers. I compared shared
costing(0001-vacuum_costing_test.patch) vs shared costing latest
patch(shared_costing_plus_patch4_v1.patch).
With shared costing base patch, I can see that delay is not in sync
compared to I/O which is resolved by applying patch
(shared_costing_plus_patch4_v1.patch).  I have also observed that total
delay is slightly reduced with  shared_costing_plus_patch4_v1.patch patch.

Below is the full testing summary:
*Test setup:*
step1) Apply parallel vacuum patch
step2) Apply 0001-vacuum_costing_test.patch patch (on the top of this
patch, delay is not in sync compared to I/O)
step3) Apply shared_costing_plus_patch4_v1.patch (delay is in sync compared
to I/O)

*Configuration settings:*
autovacuum = off
max_parallel_workers = 30
shared_buffers = 2GB
max_parallel_maintenance_workers = 20
vacuum_cost_limit = 2000
vacuum_cost_delay = 10

*Test 1: Varry indexes(2,4,6,8) but parallel workers are fixed as 4:*

Case 1) When indexes are 2:
*Without shared_costing_plus_patch4_v1.patch:*
WARNING:  worker 0 delay=120.00 total io=17931 hit=17891 miss=0 dirty=2
WARNING:  worker 1 delay=60.00 total io=17931 hit=17891 miss=0 dirty=2

*With  shared_costing_plus_patch4_v1.patch:*
WARNING:  worker 0 delay=87.78 total io=17931 hit=17891 miss=0 dirty=2
WARNING:  worker 1 delay=87.995000 total io=17931 hit=17891 miss=0 dirty=2

Case 2) When indexes are 4:
*Without shared_costing_plus_patch4_v1.patch:*
WARNING:  worker 0 delay=120.00 total io=17931 hit=17891 miss=0 dirty=2
WARNING:  worker 1 delay=80.00 total io=17931 hit=17891 miss=0 dirty=2
WARNING:  worker 2 delay=60.00 total io=17931 hit=17891 miss=0 dirty=2
WARNING:  worker 3 delay=100.00 total io=17931 hit=17891 miss=0 dirty=2

*With  shared_costing_plus_patch4_v1.patch:*
WARNING:  worker 0 delay=87.43 total io=17931 hit=17891 miss=0 dirty=2
WARNING:  worker 1 delay=87.175000 total io=17931 hit=17891 miss=0 dirty=2
WARNING:  worker 2 delay=86.34 total io=17931 hit=17891 miss=0 dirty=2
WARNING:  worker 3 delay=88.02 total io=17931 hit=17891 miss=0 dirty=2

Case 3) When indexes are 6:
*Without shared_costing_plus_patch4_v1.patch:*
WARNING:  worker 0 delay=110.00 total io=17931 hit=17891 miss=0 dirty=2
WARNING:  worker 1 delay=100.00 total io=17931 hit=17891 miss=0 dirty=2
WARNING:  worker 2 delay=160.00 total io=35862 hit=35782 miss=0 dirty=4
WARNING:  worker 3 delay=90.00 total io=17931 hit=17891 miss=0 dirty=2
WARNING:  worker 4 delay=80.00 total io=17931 hit=17891 miss=0 dirty=2

*With  shared_costing_plus_patch4_v1.patch*:
WARNING:  worker 0 delay=173.195000 total io=35862 hit=35782 miss=0 dirty=4
WARNING:  worker 1 delay=88.715000 total io=17931 hit=17891 miss=0 dirty=2
WARNING:  worker 2 delay=87.71 total io=17931 hit=17891 miss=0 dirty=2
WARNING:  worker 3 delay=86.46 total io=17931 hit=17891 miss=0 dirty=2
WARNING:  worker 4 delay=89.435000 total io=17931 hit=17891 miss=0 dirty=2

Case 4) When indexes are 8:
*Without shared_costing_plus_patch4_v1.patch:*
WARNING:  worker 0 delay=170.00 total io=35862 hit=35782 miss=0 dirty=4
WARNING:  worker 1 

Re: dropdb --force

[vignesh C]

On Wed, Nov 13, 2019 at 8:05 PM Pavel Stehule  wrote:
>
>
>
> 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
>

Thanks for working on the patch.
Few minor comments:
+Force termination of connected backends before removing the database.
+   
+   
+This will add the FORCE option to the DROP
+DATABASE command sent to the server.
+   

The above documentation can be changed similar to drop_database.sgml:
 
  Attempt to terminate all existing connections to the target database.
  It doesn't terminate if prepared transactions, active logical replication
  slots or subscriptions are present in the target database.
 
 
  This will fail if the current user has no permissions to terminate other
  connections. Required permissions are the same as with
  pg_terminate_backend, described in
  .  This will also fail if we
  are not able to terminate connections.
 

We can make the modification in the same location as earlier in the below case:
-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)" : "");
+

We can slightly rephrase the below:
+printf(_("  -f, --force   force termination of
connected backends\n"));
can be changed to:
+printf(_("  -f, --force   terminate the existing
connections to the target database forcefully\n"));

We can slightly rephrase the below:
+/* now, only FORCE option can be used, so usage is very simple */
+appendPQExpBuffer(, "DROP DATABASE %s%s%s;",
can be changed to:
+/* Generate drop db command using the options specified */
+appendPQExpBuffer(, "DROP DATABASE %s%s%s;",

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

Re: ssl passphrase callback

[Magnus Hagander]

On Wed, Nov 13, 2019 at 9:23 PM Tomas Vondra 
wrote:

> 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.
>

Yeah, that probably wouldn't hurt. It's also securely passing from more
than one perspective -- both from the "cannot be eavesdropped" (like
putting the password on the commandline for example) and the requirement
for escaping.


>
> >
> >> > 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.
>

There aren't exactly a large number of GUCs that take OS commands in total.
Consistency itself certainly has some value.

-- 
 Magnus Hagander
 Me: https://www.hagander.net/ 
 Work: https://www.redpill-linpro.com/ 

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

[Dilip Kumar]

On Thu, Nov 14, 2019 at 12:10 PM Amit Kapila  wrote:
>
> 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.
Right
>
> 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?
The idea seems reasonable to me.

Apart from this, I have another question in
0003-Issue-individual-invalidations-with-wal_level-logical.patch

@@ -543,6 +588,18 @@ RegisterSnapshotInvalidation(Oid dbId, Oid relId)
 {
  AddSnapshotInvalidationMessage(>CurrentCmdInvalidMsgs,
 dbId, relId);
+
+ /* Issue an invalidation WAL record (when wal_level=logical) */
+ if (XLogLogicalInfoActive())
+ {
+ SharedInvalidationMessage msg;
+
+ msg.sn.id = SHAREDINVALSNAPSHOT_ID;
+ msg.sn.dbId = dbId;
+ msg.sn.relId = relId;
+
+ LogLogicalInvalidations(1, , false);
+ }
 }

I am not sure why do we need to explicitly WAL log the snapshot
invalidation? because this is logged for invalidating the catalog
snapshot and for logical decoding we use HistoricSnapshot, not the
catalog snapshot.  I might be missing something?

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

Re: Optimize partial TOAST decompression

[Rushabh Lathia]

Today I noticed strange behaviour, consider the following test:

postgres@126111=#create table foo ( a text );
CREATE TABLE
postgres@126111=#insert into foo values ( repeat('PostgreSQL is the
world''s best database and leading by an Open Source Community.', 8000));
INSERT 0 1

postgres@126111=#select substring(a from 639921 for 81) from foo;
 substring
---

(1 row)

Before below commit:

commit 540f31680913b4e11f2caa40cafeca269cfcb22f
Author: Tomas Vondra 
Date:   Tue Oct 1 16:53:04 2019 +0200

Blind attempt to fix pglz_maximum_compressed_size

Commit 11a078cf87 triggered failures on big-endian machines, and the
only plausible place for an issue seems to be that TOAST_COMPRESS_SIZE
calls VARSIZE instead of VARSIZE_ANY. So try fixing that blindly.

Discussion:
https://www.postgresql.org/message-id/20191001131803.j6uin7nho7t6vxzy%40development

postgres@75761=#select substring(a from 639921 for 81) from foo;

substring

--
 PostgreSQL is the world's best database and leading by an Open Source
Community.
(1 row)

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

[Nikolay Shaplov]

В письме от четверг, 14 ноября 2019 г. 16:50:18 MSK пользователь Michael 
Paquier написал:

> > 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.

For me there is no mush sense in it, as it does not prevent us from wrong type 
casting. Indexes can use all kinds of structures for reloptions, and checking 
that this is index, will not do much.

Do you know way how to distinguish one index from another? If we can check in 
assertion this is index, and this index is spgist, then assertion will make 
sense for 100%. I just have no idea how to do it. As far as I can see it is 
not possible now.


Another issue here is, that we should to it to all indexes, not only that have 
been using StdRdOptions, but to all indexes we have. (Damn code 
inconsistency). So I guess this should go as another patch to keep it step by 
step improvements.



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

Re: Coding in WalSndWaitForWal

[Kyotaro Horiguchi]

At Wed, 13 Nov 2019 14:21:13 +0530, Amit Kapila  wrote 
in 
> 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?

I think so for the first part, but I'm not sure for the second. But it
should be avoided if it can be happen.

# the walreader's callback structure makes such things less clear :p

I remember that there was a fixed bug that logical replication code
fails to send a reply for a longer time than timeout on a very fast
connection, running through a fast path without checking the need for
reply. I couldn't find where it is, though..

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: MarkBufferDirtyHint() and LSN update

[Kyotaro Horiguchi]

At Thu, 14 Nov 2019 12:01:29 +0900, Michael Paquier  wrote 
in 
> 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.

I thought of something like that but forgot to mention. But after
thinking of that, couldn't the current code can do the same think even
though with a far small probability? Still a session with smaller hint
LSN but didn't entered the header lock section yet can be cut-in by
another session with larger hint LSN.

> for the reference attached is the trick I have used, adding an extra
> assertion check in PageSetLSN(). 

I believe that all locations where Page-LSN is set is in the same
buffer-ex-lock section with XLogInsert.. but not sure.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center