Re: Questions/Observations related to Gist vacuum

[Amit Kapila]

On Fri, Oct 18, 2019 at 10:48 AM Amit Kapila  wrote:
>
> Thanks for the test.  It shows that prior to patch the memory was
> getting leaked in TopTransactionContext during multi-pass vacuum and
> after patch, there is no leak.  I will commit the patch early next
> week unless Heikki or someone wants some more tests.
>

Pushed.

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

Cannot commit or rollback in “security definer” PL/pgSQL proc

[Bryn Llewellyn]

Here’s a cut-down version of Umair Shahid’s blog post here:

https://www.2ndquadrant.com/en/blog/postgresql-11-server-side-procedures-part-1/
 

__

create table t(k int primary key, v int not null);

create or replace procedure p()
  language plpgsql
  security invoker
as $$
begin
  insert into t(k, v) values(1, 17);
  rollback;
  insert into t(k, v) values(1, 42);
  commit;
end
$$;

call p();
select * from t order by k;
__

It runs without error and shows that the effect of “rollback” and “commit” is 
what the names of those statements tells you to expect.

The post starts with “Thanks to the work done by 2ndQuadrant contributors, we 
now have the ability to write Stored Procedures in PostgreSQL… [with] 
transaction control – allowing us to COMMIT and ROLLBACK inside procedures.”. I 
believe that Umair is referring to work done by Peter Eisentraut.

But simply change “security invoker” to “security definer” and rerun the test. 
You get the notorious error “2D000: invalid transaction termination”.

Please tell me that this is a plain bug—and not the intended semantics.

Re: dropdb --force

[Pavel Stehule]

po 21. 10. 2019 v 7:11 odesílatel Amit Kapila 
napsal:

> On Sun, Oct 20, 2019 at 2:06 AM Pavel Stehule 
> wrote:
> >
> > so 19. 10. 2019 v 12:37 odesílatel Amit Kapila 
> napsal:
> >>
> >> On Fri, Oct 4, 2019 at 9:54 PM Pavel Stehule 
> wrote:
> >> >
> >> >>
> >> >> Can we add few tests for this feature.
> >> >
> >> > there are not any other test for DROP DATABASE
> >> >
> >>
> >> I think there are no dedicated tests for it but in a few tests, we use
> >> it like in contrib\sepgsql\sql\alter.sql.  I am not sure if we can
> >> write a predictable test for force option because it will never be
> >> guaranteed to drop the database in the presence of other active
> >> sessions.
> >
> >
> > done - I push tests to /tests/regress/psql.sql
> >
> >>
> >> Few more comments:
> >> -
> >>
> >> 2.
> >> TerminateOtherDBBackends()
> >> {
> >> ..
> >> + /* We know so we have all necessary rights now */
> >> + foreach (lc, pids)
> >> + {
> >> + int pid = lfirst_int(lc);
> >> + PGPROC*proc = BackendPidGetProc(pid);
> >> +
> >> + if (proc != NULL)
> >> + {
> >> + /* If we have setsid(), signal the backend's whole process group */
> >> +#ifdef HAVE_SETSID
> >> + (void) kill(-pid, SIGTERM);
> >> +#else
> >> + (void) kill(pid, SIGTERM);
> >> +#endif
> >> + }
> >> + }
> >> +
> >> + /* sleep 100ms */
> >> + pg_usleep(100 * 1000L);
> >> ..
> >> }
> >>
> >> So here we are sending SIGTERM to all the processes and wait for 100ms
> >> to allow them to exit.  Have you tested this with many processes?  I
> >> think we can create 100~500 sessions or maybe more to the database
> >> being dropped and see what is behavior?  One thing to notice is that
> >> in function CountOtherDBBackends() we are sending SIGTERM to 10
> >> autovacuum processes at-a-time.  That has been introduced in commit
> >> 4abd7b49f1e9, but the reason for the same is not mentioned.  I am not
> >> sure if it is to avoid sending SIGTERM to many processes in quick
> >> succession.
> >
> >
> > I tested it on linux Linux nemesis
> >
> > 5.3.6-300.fc31.x86_64 #1 SMP Mon Oct 14 12:26:42 UTC 2019 x86_64 x86_64
> x86_64 GNU/Linux
> >
> > Tested with 1800 connections without any problem
> >
>
> When you say 'without any problem', do you mean to say that the drop
> database is successful?  In your tests were those sessions idle?  If
> so, I think we should try with busy sessions.  Also, if you write any
> script to do these tests, kindly share the same so that others can
> also repeat those tests.
>
>
sessions was active - but the function pg_sleep was called. Drop table
(mainly logout of these users was successful).

I had a script just

for i in {1..1000}; do (psql -c "select pg_sleep(1000)" omega &> /dev/null
&); done

I'll try to do some experiments - unfortunately,I have not a hw where I can
test very large number of connections.

But surely I can use pg_bench, and I can check pg_bench load



> >(under low load (only pg_sleep was called).
> >
>
> I guess this is also possible because immediately after
> TerminateOtherDBBackends, we call CountOtherDBBackends which again
> give 5s to allow active connections to die. I am wondering why not we
> call CountOtherDBBackends from TerminateOtherDBBackends after sending
> the SIGTERM to all the sessions that are connected to the database
> being dropped?  Currently, it looks odd that first, we wait for 100ms
> after sending the signal and then separately wait for 5s in another
> function.
>

I'll look to this part, but I don't think so it is bad. 5s is maximum, not
minimum of waiting. So if sigterm is successful in first 100ms, then
CountOtherDBBackends doesn't add any time. If not, then it dynamically
waiting.

If we don't wait in TerminateOtherDBBackends, then probably there should be
necessary some cycles inside CountOtherDBBackends. I think so code like is
correct

1. send SIGTERM to target processes
2. put some time to processes for logout (100ms)
3. check in loop (max 5 sec) on logout of all processes

Maybe my feeling is wrong, but I think so it is good to wait few time
instead to call CountOtherDBBackends immediately - the first iteration
should to fail, and then first iteration is useless without chance on
success.


> Other comments:
> 1.
> diff --git a/src/test/regress/sql/psql.sql b/src/test/regress/sql/psql.sql
> index 26a0bcf718..c8f545be18 100644
> --- a/src/test/regress/sql/psql.sql
> +++ b/src/test/regress/sql/psql.sql
> @@ -1182,3 +1182,11 @@ drop role regress_partitioning_role;
>
>  -- \d on toast table (use pg_statistic's toast table, which has a known
> name)
>  \d pg_toast.pg_toast_2619
> +
> +--
> +-- DROP DATABASE FORCE test of syntax (should not to raise syntax error)
> +--
> +drop database not_exists (force);
> +drop database not_exists with (force);
> +drop database if exists not_exists (force);
> +drop database if exists not_exists with (force);
>
> I don't think psql.sql is the right place to add such tests.
> Actually, there doesn't appear to be any database specific 

Re: Remove obsolete information schema tables

[Michael Paquier]

On Sun, Oct 20, 2019 at 10:01:09AM +0200, Peter Eisentraut wrote:
> On 2019-10-17 09:44, Michael Paquier wrote:
> > I have a question here.  Per the notes in information_schema.sql,
> > SQL_SIZING_PROFILES has been removed in SQL:2011,
> 
> OK, we can remove that one as well.  New patch attached.

Looks fine.

>> attributes.isnullable and DOMAIN_UDT_USAGE in SQL:2003~.  Would it
>> make sense to cleanup those ones?
> 
> OK, I'll look into those, but it seems like a separate undertaking.  We
> don't always remove things just because they were dropped by the SQL
> standard.

But that's the same kind of cleanup you do here.  What's the
difference with DOMAIN_UDT_USAGE, which is mentioned as removed from
SQL:2003?
--
Michael


signature.asc
Description: PGP signature

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

[Dilip Kumar]

On Fri, Oct 18, 2019 at 5:32 PM Amit Kapila  wrote:

I have replied to some of your questions inline.  I will work on the
remaining comments and post the patch for the same.

> > >
> > > Sure, I wasn't really proposing to adding all stats from that patch,
> > > including those related to streaming.  We need to extract just those
> > > related to spilling. And yes, it needs to be moved right after 0001.
> > >
> > I have extracted the spilling related code to a separate patch on top
> > of 0001.  I have also fixed some bugs and review comments and attached
> > as a separate patch.  Later I can merge it to the main patch if you
> > agree with the changes.
> >
>
> Few comments
> -
> 0001-Add-logical_decoding_work_mem-to-limit-ReorderBuffer
> 1.
> + {
> + {"logical_decoding_work_mem", PGC_USERSET, RESOURCES_MEM,
> + gettext_noop("Sets the maximum memory to be used for logical decoding."),
> + gettext_noop("This much memory can be used by each internal "
> + "reorder buffer before spilling to disk or streaming."),
> + GUC_UNIT_KB
> + },
>
> I think we can remove 'or streaming' from above sentence for now.  We
> can add it later with later patch where streaming will be allowed.
>
> 2.
> @@ -206,6 +206,18 @@ CREATE SUBSCRIPTION  class="parameter">subscription_name   
>  
> 
> +
> +   
> +work_mem (integer)
> +
> + 
> +  Limits the amount of memory used to decode changes on the
> +  publisher.  If not specified, the publisher will use the default
> +  specified by logical_decoding_work_mem. When
> +  needed, additional data are spilled to disk.
> + 
> +
> +   
>
> It is not clear why we need this parameter at least with this patch?
> I have raised this multiple times [1][2].
>
> bugs_and_review_comments_fix
> 1.
> },
>   _decoding_work_mem,
> - -1, -1, MAX_KILOBYTES,
> - check_logical_decoding_work_mem, NULL, NULL
> + 65536, 64, MAX_KILOBYTES,
> + NULL, NULL, NULL
>
> I think the default value should be 1MB similar to
> maintenance_work_mem.  The same was true before this change.
>
> 2. -#logical_decoding_work_mem = 64MB # min 1MB, or -1 to use
> maintenance_work_mem
> +i#logical_decoding_work_mem = 64MB # min 64kB
>
> It seems the 'i' is a leftover character in the above change.  Also,
> change the default value considering the previous point.
>
> 3.
> @@ -2479,7 +2480,7 @@ ReorderBufferSerializeTXN(ReorderBuffer *rb,
> ReorderBufferTXN *txn)
>
>   /* update the statistics */
>   rb->spillCount += 1;
> - rb->spillTxns += txn->serialized ? 1 : 0;
> + rb->spillTxns += txn->serialized ? 0 : 1;
>   rb->spillBytes += size;
>
> Why is this change required?  Shouldn't we increase the spillTxns
> count only when the txn is serialized?

Prior to this change it was increasing the rb->spillTxns, every time
we try to serialize the changes of the transaction.  Now, only we
increase first time when it is not yet serialized.

> 0002-Track-statistics-for-spilling
> 1.
> +
> + spill_txns
> + integer
> + Number of transactions spilled to disk after the memory used by
> +  logical decoding exceeds logical_work_mem. The
> +  counter gets incremented both for toplevel transactions and
> +  subtransactions.
> +  
> +
>
> The parameter name is wrong here. /logical_work_mem/logical_decoding_work_mem
>
> 2.
> +
> + spill_txns
> + integer
> + Number of transactions spilled to disk after the memory used by
> +  logical decoding exceeds logical_work_mem. The
> +  counter gets incremented both for toplevel transactions and
> +  subtransactions.
> +  
> +
> +
> + spill_count
> + integer
> + Number of times transactions were spilled to disk. Transactions
> +  may get spilled repeatedly, and this counter gets incremented on every
> +  such invocation.
> +  
> +
> +
> + spill_bytes
> + integer
> + Amount of decoded transaction data spilled to disk.
> +  
> +
>
> In all the above cases, the explanation text starts immediately after
>  tag, but the general coding practice is to start from the next
> line, see the explanation of nearby parameters.
>
> It seems these parameters are added in pg-stat-wal-receiver-view in
> the docs, but in code, it is present as part of pg_stat_replication.
> It seems doc needs to be updated.  Am, I missing something?
>
> 3.
> ReorderBufferSerializeTXN()
> {
> ..
> /* update the statistics */
> rb->spillCount += 1;
> rb->spillTxns += txn->serialized ? 0 : 1;
> rb->spillBytes += size;
>
> Assert(spilled == txn->nentries_mem);
> Assert(dlist_is_empty(>changes));
> txn->nentries_mem = 0;
> txn->serialized = true;
> ..
> }
>
> I am not able to understand the above code.  We are setting the
> serialized parameter a few lines after we check it and increment the
> spillTxns count. Can you please explain it?

Basically, when the first time we attempt to serialize a 

Re: dropdb --force

[Amit Kapila]

On Sun, Oct 20, 2019 at 2:06 AM Pavel Stehule  wrote:
>
> so 19. 10. 2019 v 12:37 odesílatel Amit Kapila  
> napsal:
>>
>> On Fri, Oct 4, 2019 at 9:54 PM Pavel Stehule  wrote:
>> >
>> >>
>> >> Can we add few tests for this feature.
>> >
>> > there are not any other test for DROP DATABASE
>> >
>>
>> I think there are no dedicated tests for it but in a few tests, we use
>> it like in contrib\sepgsql\sql\alter.sql.  I am not sure if we can
>> write a predictable test for force option because it will never be
>> guaranteed to drop the database in the presence of other active
>> sessions.
>
>
> done - I push tests to /tests/regress/psql.sql
>
>>
>> Few more comments:
>> -
>>
>> 2.
>> TerminateOtherDBBackends()
>> {
>> ..
>> + /* We know so we have all necessary rights now */
>> + foreach (lc, pids)
>> + {
>> + int pid = lfirst_int(lc);
>> + PGPROC*proc = BackendPidGetProc(pid);
>> +
>> + if (proc != NULL)
>> + {
>> + /* If we have setsid(), signal the backend's whole process group */
>> +#ifdef HAVE_SETSID
>> + (void) kill(-pid, SIGTERM);
>> +#else
>> + (void) kill(pid, SIGTERM);
>> +#endif
>> + }
>> + }
>> +
>> + /* sleep 100ms */
>> + pg_usleep(100 * 1000L);
>> ..
>> }
>>
>> So here we are sending SIGTERM to all the processes and wait for 100ms
>> to allow them to exit.  Have you tested this with many processes?  I
>> think we can create 100~500 sessions or maybe more to the database
>> being dropped and see what is behavior?  One thing to notice is that
>> in function CountOtherDBBackends() we are sending SIGTERM to 10
>> autovacuum processes at-a-time.  That has been introduced in commit
>> 4abd7b49f1e9, but the reason for the same is not mentioned.  I am not
>> sure if it is to avoid sending SIGTERM to many processes in quick
>> succession.
>
>
> I tested it on linux Linux nemesis
>
> 5.3.6-300.fc31.x86_64 #1 SMP Mon Oct 14 12:26:42 UTC 2019 x86_64 x86_64 
> x86_64 GNU/Linux
>
> Tested with 1800 connections without any problem
>

When you say 'without any problem', do you mean to say that the drop
database is successful?  In your tests were those sessions idle?  If
so, I think we should try with busy sessions.  Also, if you write any
script to do these tests, kindly share the same so that others can
also repeat those tests.

>(under low load (only pg_sleep was called).
>

I guess this is also possible because immediately after
TerminateOtherDBBackends, we call CountOtherDBBackends which again
give 5s to allow active connections to die. I am wondering why not we
call CountOtherDBBackends from TerminateOtherDBBackends after sending
the SIGTERM to all the sessions that are connected to the database
being dropped?  Currently, it looks odd that first, we wait for 100ms
after sending the signal and then separately wait for 5s in another
function.

Other comments:
1.
diff --git a/src/test/regress/sql/psql.sql b/src/test/regress/sql/psql.sql
index 26a0bcf718..c8f545be18 100644
--- a/src/test/regress/sql/psql.sql
+++ b/src/test/regress/sql/psql.sql
@@ -1182,3 +1182,11 @@ drop role regress_partitioning_role;

 -- \d on toast table (use pg_statistic's toast table, which has a known name)
 \d pg_toast.pg_toast_2619
+
+--
+-- DROP DATABASE FORCE test of syntax (should not to raise syntax error)
+--
+drop database not_exists (force);
+drop database not_exists with (force);
+drop database if exists not_exists (force);
+drop database if exists not_exists with (force);

I don't think psql.sql is the right place to add such tests.
Actually, there doesn't appear to be any database specific test file.
However, if we want to add to any existing file, then maybe
drop_if_exits.sql could be a better place for these tests as compare
to psql.sql.

2.
+ if (nprepared > 0)
+ ereport(ERROR,
+ (errcode(ERRCODE_OBJECT_IN_USE),
+ errmsg_plural("database \"%s\" is used by %d prepared transaction",
+"database \"%s\" is used by %d prepared transactions",
+nprepared,
+get_database_name(databaseId), nprepared)));

I think it is better if we mimic exactly what we have in the failure
of  CountOtherDBBackends.

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

Re: Fix of fake unlogged LSN initialization

[Michael Paquier]

On Sat, Oct 19, 2019 at 05:03:00AM +, tsunakawa.ta...@fujitsu.com wrote:
> The attached trivial patch fixes the initialization of the fake
> unlogged LSN.  Currently, BootstrapXLOG() in initdb sets the initial
> fake unlogged LSN to FirstNormalUnloggedLSN (=1000), but the
> recovery and pg_resetwal sets it to 1.  The patch modifies the
> latter two cases to match initdb. 
> 
> I don't know if this do actual harm, because the description of
> FirstNormalUnloggedLSN doesn't give me any idea. 

From xlogdefs.h added by 9155580:
/*
 * First LSN to use for "fake" LSNs.
 *
 * Values smaller than this can be used for special per-AM purposes.
 */
#define FirstNormalUnloggedLSN  ((XLogRecPtr) 1000)

So it seems to me that you have caught a bug here, and that we had
better back-patch to v12 so as recovery and pg_resetwal don't mess up
with AMs using lower values than that.
--
Michael


signature.asc
Description: PGP signature

Re: Missing error_context_stack = NULL in AutoVacWorkerMain()

[Tom Lane]

Michael Paquier  writes:
> On Mon, Oct 21, 2019 at 12:47:40AM -0400, Tom Lane wrote:
>> This seems like a real and possibly serious bug to me.  Backend sigsetjmp
>> callers *must* clear error_context_stack (or restore it to a previous
>> value), because if it isn't NULL it's surely pointing at garbage, ie a
>> local variable that's no longer part of the valid stack.

> Sure.  From my recollection of memories we never set it in autovacuum
> code paths (including index entry deletions), so I don't think that we
> have an actual live bug here.

Uh ... what about, say, auto-analyze on an expression index?  That
could call user-defined PL functions and thus reach just about all
of the backend.

regards, tom lane

Re: Missing error_context_stack = NULL in AutoVacWorkerMain()

[Michael Paquier]

On Mon, Oct 21, 2019 at 12:47:40AM -0400, Tom Lane wrote:
> This seems like a real and possibly serious bug to me.  Backend sigsetjmp
> callers *must* clear error_context_stack (or restore it to a previous
> value), because if it isn't NULL it's surely pointing at garbage, ie a
> local variable that's no longer part of the valid stack.

Sure.  From my recollection of memories we never set it in autovacuum
code paths (including index entry deletions), so I don't think that we
have an actual live bug here.

> The issue might be argued to be insignificant because the autovacuum
> worker is just going to do proc_exit anyway.  But if it encountered
> another error during proc_exit, elog.c might try to invoke error
> callbacks using garbage callback data.
> 
> In short, I think we'd better back-patch too.

Okay, no objections to back-patch.
--
Michael


signature.asc
Description: PGP signature

Re: Missing error_context_stack = NULL in AutoVacWorkerMain()

[Tom Lane]

I wrote:
> The issue might be argued to be insignificant because the autovacuum
> worker is just going to do proc_exit anyway.  But if it encountered
> another error during proc_exit, elog.c might try to invoke error
> callbacks using garbage callback data.

Oh --- looking closer, proc_exit itself will clear error_context_stack
before doing much.  So a problem would only occur if we suffered an error
during EmitErrorReport, which seems somewhat unlikely.  Still, it's bad
that this code isn't like all the others.  There's certainly no downside
to clearing the pointer.

regards, tom lane

Re: Missing error_context_stack = NULL in AutoVacWorkerMain()

[Tom Lane]

Michael Paquier  writes:
> On Fri, Oct 18, 2019 at 05:55:32PM -0700, Ashwin Agrawal wrote:
>> I am not sure if this causes any potential problems or not, but for
>> consistency of code seems we are missing below. All other places in code
>> where sigsetjmp() exists for top level handling has error_context_stack set
>> to NULL.

> Resetting error_context_stack prevents calling any callbacks which may
> be set.  These would not be much useful in this context anyway, and
> visibly that's actually not an issue with the autovacuum code so far
> (I don't recall seeing a custom callback setup in this area, but I may
> have missed something).  So fixing it would be a good thing actually,
> on HEAD.

> Any thoughts from others?

This seems like a real and possibly serious bug to me.  Backend sigsetjmp
callers *must* clear error_context_stack (or restore it to a previous
value), because if it isn't NULL it's surely pointing at garbage, ie a
local variable that's no longer part of the valid stack.

The issue might be argued to be insignificant because the autovacuum
worker is just going to do proc_exit anyway.  But if it encountered
another error during proc_exit, elog.c might try to invoke error
callbacks using garbage callback data.

In short, I think we'd better back-patch too.

regards, tom lane

Re: Fix most -Wundef warnings

[Andrew Gierth]

> "Mark" == Mark Dilger  writes:

 Mark> I tried briefly to download this project from pgfoundry without
 Mark> success. Do you have a copy of the relevant code where you can
 Mark> see how this gets defined, and can you include it in a reply?

I have a backup of the CVS from the pgfoundry version, but the thing is
so obsolete that I had never bothered converting it to git; it hasn't
been touched in 10 years.

The Makefile had this:

PG_CPPFLAGS = -DHSTORE_IS_HSTORE_NEW

The only possible use for this code is if someone were to discover an
old 8.4 install with an old hstore-new module in use. I think the
chances of this are small enough not to be of much concern.

I have put up a CVS->Git conversion for the benefit of software
archaeologists only at: https://github.com/RhodiumToad/hstore-ancient

-- 
Andrew (irc:RhodiumToad)

Re: Remove obsolete options for createuser

[Michael Paquier]

On Sat, Oct 19, 2019 at 03:34:56PM +0300, Alexander Lakhin wrote:
> I've noticed that the createuser utility supports two undocumented
> options (--adduser, --no-adduser), that became obsolete in 2005.
> I believe that their existence should come to end someday (maybe
> today?). The patch to remove them is attached.

The commit in question is 8ae0d47 from 2005.  So let's remove it.  It
is not even documented for ages.

Perhaps somebody thinks it is not a good idea?
--
Michael


signature.asc
Description: PGP signature

Re: Missing error_context_stack = NULL in AutoVacWorkerMain()

[Michael Paquier]

On Fri, Oct 18, 2019 at 05:55:32PM -0700, Ashwin Agrawal wrote:
> I am not sure if this causes any potential problems or not, but for
> consistency of code seems we are missing below. All other places in code
> where sigsetjmp() exists for top level handling has error_context_stack set
> to NULL.

Resetting error_context_stack prevents calling any callbacks which may
be set.  These would not be much useful in this context anyway, and
visibly that's actually not an issue with the autovacuum code so far
(I don't recall seeing a custom callback setup in this area, but I may
have missed something).  So fixing it would be a good thing actually,
on HEAD.

Any thoughts from others?
--
Michael


signature.asc
Description: PGP signature

Re: Ordering of header file inclusion

[Tom Lane]

Amit Kapila  writes:
> On Sun, Oct 20, 2019 at 10:58 PM vignesh C  wrote:
>> Should we make this changes only in master branch or should we make in
>> back branches also.

> I am in favor of doing this only for HEAD, but I am fine if others
> want to see for back branches as well and you can prepare the patches
> for the same.

There is no good reason to back-patch this.

regards, tom lane

Re: Ordering of header file inclusion

[Amit Kapila]

On Sun, Oct 20, 2019 at 10:58 PM vignesh C  wrote:
>
> On Thu, Oct 17, 2019 at 4:44 PM Amit Kapila  wrote:
> >
> >
> > I haven't reviewed it completely, but generally, the changes seem to
> > be fine.  Please see if you can be consistent in extra space between
> > includes.  Kindly check the same throughout the patch.
> >
> Thanks for reviewing the patch.
> I have made an updated patch with comments you have suggested.
> I have split the patch into 3 patches so that the review can be simpler.
> This patch also includes the changes suggested by Peter & Andres.
> I had just seen seen Tom Lane's suggestions regarding submodule header
> file, this patch contains fix based on Andres suggestions. Let me know
> if that need to be changed, I can update it.
>

AFAICS, none of Andres or Tom seems to be in favor of separating
module headers.  I am also not sure if we should try to make sure of
that in every case.

> Should we make this changes only in master branch or should we make in
> back branches also.
>

I am in favor of doing this only for HEAD, but I am fine if others
want to see for back branches as well and you can prepare the patches
for the same.

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

Re: Fix most -Wundef warnings

[Mark Dilger]

On 10/15/19 5:23 AM, Andrew Gierth wrote:

"Mark" == Mark Dilger  writes:


  >> +#ifdef HSTORE_IS_HSTORE_NEW

  Mark> Checking the current sources, git history, and various older
  Mark> commits, I did not find where HSTORE_IS_HSTORE_NEW was ever
  Mark> defined.

In contrib/hstore, it never was.

The current version of contrib/hstore had a brief life as a separate
extension module called hstore-new, which existed to backport its
functionality into 8.4. The data format for hstore-new was almost
identical to the new contrib/hstore one (and thus different from the old
contrib/hstore), and changed at one point before its final release, so
there were four possible upgrade paths as explained in the comments.

The block comment with the most pertinent explanation seems to have
been a victim of pgindent, but the relevant part is this:

  * [...] So the upshot of all this
  * is that we can treat all the edge cases as "new" if we're being built
  * as hstore-new, and "old" if we're being built as contrib/hstore.

So, HSTORE_IS_HSTORE_NEW was defined if you were building a pgxs module
called "hstore-new" (which was distributed separately on pgfoundry but
the C code was the same), and not if you're building "hstore" (whether
an in or out of tree build).


I don't really dispute your claim, but it doesn't unambiguously follow 
from the wording of the comment.  The part that tripped me up while 
reviewing Peter's patch is that he changed the preprocessor logic to use 
#ifdef rather than #if, implying that he believes HSTORE_IS_HSTORE_NEW 
will only be defined when true, and undefined when false, rather than 
something like:


  #if OLD_STUFF
  #define HSTORE_IS_HSTORE_NEW 0
  #else
  #define HSTORE_IS_HSTORE_NEW 1
  #endif

which is admittedly a less common coding pattern than only defining it 
when true, but the choice of #if rather than #ifdef in the original 
sources might have been intentional.


I tried briefly to download this project from pgfoundry without success. 
 Do you have a copy of the relevant code where you can see how this 
gets defined, and can you include it in a reply?


Thanks,

mark

Re: libpq: Fix wrong connection status on invalid "connect_timeout"

[Michael Paquier]

On Fri, Oct 18, 2019 at 02:01:23PM +0200, Lars Kanis wrote:
> Am 18.10.19 um 05:06 schrieb Michael Paquier:
>> So attached is a patch to skip trailing whitespaces as well,
>> which also fixes the issue with ECPG.  I have refactored the parsing
>> logic a bit while on it.  The comment at the top of parse_int_param()
>> needs to be reworked a bit more.
> 
> I tested this and it looks good to me. Maybe you could omit some
> redundant 'end' checks, as in the attached patch. Or was your intention
> to verify non-NULL 'end'?

Yes.  Here are the connection patterns I have tested.  These now pass:
'postgresql:///postgres?host=/tmp=5432  =postgres'
'postgresql:///postgres?host=/tmp=  5432=postgres'
And these fail (overflow on third one):
'postgresql:///postgres?host=/tmp=5432 s =postgres'
'postgresql:///postgres?host=/tmp= s 5432=postgres'
'postgresql:///postgres?host=/tmp=  50=postgres'

Before the patch any trailing characters caused a failures even if
there were just whitespaces as trailing characters (first case
listed).

>> Perhaps we could add directly regression
>> tests for libpq.  I'll start a new thread about that once we are done
>> here, the topic is larger.
> 
> We have around 650 tests on ruby-pg to ensure everything runs as
> expected and I always wondered how the API of libpq is being verified.

For advanced test scenarios like connection handling, we use perl's
TAP tests.  The situation regarding libpq-related testing is a bit
messy though.  We have some tests in src/test/recovery/ for a couple
of things, and we should have more things to stress anything related
to the protocol (say message injection, etc.).

I'll try to start a new thread about that with a patch adding some
basics for discussion.

I have applied the parsing fix and your fix as two separate commits as
these are at the end two separate bugs, then back-patched down to v12.
Ed has been credited for the report, and I have marked the author as
you, Lars.  Thanks!
--
Michael


signature.asc
Description: PGP signature

回复：Bug about drop index concurrently

[李杰(慎追)]

Thanks for the quick reply.
And sorry I haven’t got back to you sooner .

I have seen this backtrace in the core file, and I also looked at the bug in 
the standby because there is no lock in the drop index concurrently.

However, when our business will perform a large number of queries in the 
standby, this problem will occur more frequently. So we are trying to solve 
this problem, and the solution we are currently dealing with is to ban it.

Of course, we considered applying the method of waiting to detect the query 
lock on the master to the standby, but worried about affecting the standby 
application log delay, so we gave up that.

If you have a better solution in the future, please push it to the new version, 
or email it, thank you very much.

regards.

adger.


--
发件人：Tomas Vondra 
发送时间：2019年10月19日(星期六) 02:00
收件人：李杰(慎追) 
抄　送：pgsql-hackers 
主　题：Re: Bug about drop index concurrently

Hi,

I can trivially reproduce this - it's enough to create a master-standby
setup, and then do this on the master

  CREATE TABLE t (a int, b int);
  INSERT INTO t SELECT i, i FROM generate_series(1,1) s(i);

and run pgbench with this script

  DROP INDEX CONCURRENTLY IF EXISTS t_a_idx;
  CREATE INDEX t_a_idx ON t(a);

while on the standby there's another pgbench running this script

  EXPLAIN ANALYZE SELECT * FROM t WHERE a = 1;

and it fails pretty fast for me. With an extra assert(false) added to
src/backend/access/common/relation.c I get a backtrace like this:

Program terminated with signal SIGABRT, Aborted.
#0  0x7c32e458fe35 in raise () from /lib64/libc.so.6
Missing separate debuginfos, use: dnf debuginfo-install 
glibc-2.29-22.fc30.x86_64
(gdb) bt
#0  0x7c32e458fe35 in raise () from /lib64/libc.so.6
#1  0x7c32e457a895 in abort () from /lib64/libc.so.6
#2  0x00a58579 in ExceptionalCondition (conditionName=0xacd9bc 
"!(0)", errorType=0xacd95b "FailedAssertion", fileName=0xacd950 "relation.c", 
lineNumber=64) at assert.c:54
#3  0x0048d1bd in relation_open (relationId=38216, lockmode=1) at 
relation.c:64
#4  0x005082e4 in index_open (relationId=38216, lockmode=1) at 
indexam.c:130
#5  0x0080ac3f in get_relation_info (root=0x21698b8, 
relationObjectId=16385, inhparent=false, rel=0x220ce60) at plancat.c:196
#6  0x008118c6 in build_simple_rel (root=0x21698b8, relid=1, 
parent=0x0) at relnode.c:292
#7  0x007d485d in add_base_rels_to_query (root=0x21698b8, 
jtnode=0x2169478) at initsplan.c:114
#8  0x007d48a3 in add_base_rels_to_query (root=0x21698b8, 
jtnode=0x21693e0) at initsplan.c:122
#9  0x007d8fad in query_planner (root=0x21698b8, 
qp_callback=0x7ded97 , qp_extra=0x7fffa4834f10) at 
planmain.c:168
#10 0x007dc316 in grouping_planner (root=0x21698b8, 
inheritance_update=false, tuple_fraction=0) at planner.c:2048
#11 0x007da7ca in subquery_planner (glob=0x220d078, 
parse=0x2168f78, parent_root=0x0, hasRecursion=false, tuple_fraction=0) at 
planner.c:1012
#12 0x007d942c in standard_planner (parse=0x2168f78, 
cursorOptions=256, boundParams=0x0) at planner.c:406
#13 0x007d91e8 in planner (parse=0x2168f78, cursorOptions=256, 
boundParams=0x0) at planner.c:275
#14 0x008e1b0d in pg_plan_query (querytree=0x2168f78, 
cursorOptions=256, boundParams=0x0) at postgres.c:878
#15 0x00658683 in ExplainOneQuery (query=0x2168f78, 
cursorOptions=256, into=0x0, es=0x220cd90, queryString=0x21407b8 "explain 
analyze select * from t where a = 1;", params=0x0, queryEnv=0x0) at 
explain.c:367
#16 0x00658386 in ExplainQuery (pstate=0x220cc28, stmt=0x2141728, 
queryString=0x21407b8 "explain analyze select * from t where a = 1;", 
params=0x0, queryEnv=0x0, dest=0x220cb90) at explain.c:255
#17 0x008ea218 in standard_ProcessUtility (pstmt=0x21425c0, 
queryString=0x21407b8 "explain analyze select * from t where a = 1;", 
context=PROCESS_UTILITY_TOPLEVEL, params=0x0, queryEnv=0x0, dest=0x220cb90,
completionTag=0x7fffa48355c0 "") at utility.c:675
#18 0x008e9a45 in ProcessUtility (pstmt=0x21425c0, 
queryString=0x21407b8 "explain analyze select * from t where a = 1;", 
context=PROCESS_UTILITY_TOPLEVEL, params=0x0, queryEnv=0x0, dest=0x220cb90,
completionTag=0x7fffa48355c0 "") at utility.c:360
#19 0x008e8a0c in PortalRunUtility (portal=0x219c278, 
pstmt=0x21425c0, isTopLevel=true, setHoldSnapshot=true, dest=0x220cb90, 
completionTag=0x7fffa48355c0 "") at pquery.c:1175
#20 0x008e871a in FillPortalStore (portal=0x219c278, 
isTopLevel=true) at pquery.c:1035
#21 0x008e8075 in PortalRun (portal=0x219c278, 
count=9223372036854775807, isTopLevel=true, run_once=true, dest=0x21efb90, 
altdest=0x21efb90, completionTag=0x7fffa48357b0 "") at pquery.c:765
#22 0x008e207c in 

Re: [HACKERS] Block level parallel vacuum

[Masahiko Sawada]

On Fri, Oct 18, 2019 at 3:48 PM Dilip Kumar  wrote:
>
> On Fri, Oct 18, 2019 at 11:25 AM Amit Kapila  wrote:
> >
> > On Fri, Oct 18, 2019 at 8:45 AM Dilip Kumar  wrote:
> > >
> > > On Thu, Oct 17, 2019 at 4:00 PM Amit Kapila  
> > > wrote:
> > > >
> > > > On Thu, Oct 17, 2019 at 3:25 PM Dilip Kumar  
> > > > wrote:
> > > > >
> > > > > On Thu, Oct 17, 2019 at 2:12 PM Masahiko Sawada 
> > > > >  wrote:
> > > > > >
> > > > > > On Thu, Oct 17, 2019 at 5:30 PM Amit Kapila 
> > > > > >  wrote:
> > > > > > >
> > > > > > > Another point in this regard is that the user anyway has an 
> > > > > > > option to
> > > > > > > turn off the cost-based vacuum.  By default, it is anyway 
> > > > > > > disabled.
> > > > > > > So, if the user enables it we have to provide some sensible 
> > > > > > > behavior.
> > > > > > > If we can't come up with anything, then, in the end, we might 
> > > > > > > want to
> > > > > > > turn it off for a parallel vacuum and mention the same in docs, 
> > > > > > > but I
> > > > > > > think we should try to come up with a solution for it.
> > > > > >
> > > > > > I finally got your point and now understood the need. And the idea I
> > > > > > proposed doesn't work fine.
> > > > > >
> > > > > > So you meant that all workers share the cost count and if a parallel
> > > > > > vacuum worker increase the cost and it reaches the limit, does the
> > > > > > only one worker sleep? Is that okay even though other parallel 
> > > > > > workers
> > > > > > are still running and then the sleep might not help?
> > > > > >
> > > >
> > > > Remember that the other running workers will also increase
> > > > VacuumCostBalance and whichever worker finds that it becomes greater
> > > > than VacuumCostLimit will reset its value and sleep.  So, won't this
> > > > make sure that overall throttling works the same?
> > > >
> > > > > I agree with this point.  There is a possibility that some of the
> > > > > workers who are doing heavy I/O continue to work and OTOH other
> > > > > workers who are doing very less I/O might become the victim and
> > > > > unnecessarily delay its operation.
> > > > >
> > > >
> > > > Sure, but will it impact the overall I/O?  I mean to say the rate
> > > > limit we want to provide for overall vacuum operation will still be
> > > > the same.  Also, isn't a similar thing happens now also where heap
> > > > might have done a major portion of I/O but soon after we start
> > > > vacuuming the index, we will hit the limit and will sleep.
> > >
> > > Actually, What I meant is that the worker who performing actual I/O
> > > might not go for the delay and another worker which has done only CPU
> > > operation might pay the penalty?  So basically the worker who is doing
> > > CPU intensive operation might go for the delay and pay the penalty and
> > > the worker who is performing actual I/O continues to work and do
> > > further I/O.  Do you think this is not a practical problem?
> > >
> >
> > I don't know.  Generally, we try to delay (if required) before
> > processing (read/write) one page which means it will happen for I/O
> > intensive operations, so I am not sure if the point you are making is
> > completely correct.
>
> Ok, I agree with the point that we are checking it only when we are
> doing the I/O operation.  But, we also need to consider that each I/O
> operations have a different weightage.  So even if we have a delay
> point at I/O operation there is a possibility that we might delay the
> worker which is just performing read buffer with page
> hit(VacuumCostPageHit).  But, the other worker who is actually
> dirtying the page(VacuumCostPageDirty = 20) continue the work and do
> more I/O.
>
> >
> > > Stepping back a bit,  OTOH, I think that we can not guarantee that the
> > > one worker who has done more I/O will continue to do further I/O and
> > > the one which has not done much I/O will not perform more I/O in
> > > future.  So it might not be too bad if we compute shared costs as you
> > > suggested above.
> > >
> >
> > I am thinking if we can write the patch for both the approaches (a.
> > compute shared costs and try to delay based on that, b. try to divide
> > the I/O cost among workers as described in the email above[1]) and do
> > some tests to see the behavior of throttling, that might help us in
> > deciding what is the best strategy to solve this problem, if any.
> > What do you think?
>
> I agree with this idea.  I can come up with a POC patch for approach
> (b).  Meanwhile, if someone is interested to quickly hack with the
> approach (a) then we can do some testing and compare.  Sawada-san,
> by any chance will you be interested to write POC with approach (a)?

Yes, I will try to write the PoC patch with approach (a).

Regards,

--
Masahiko Sawada

Re: configure fails for perl check on CentOS8

[Tom Lane]

Andrew Dunstan  writes:
> On 10/20/19 1:23 PM, Tom Lane wrote:
>> The right way to fix it, likely, is to add CFLAGS_SL while performing this
>> particular autoconf test, as that would replicate the switches used for
>> plperl (and it turns out that adding -fPIC does solve this problem).
>> But the configure script doesn't currently know about CFLAGS_SL, so we'd
>> have to do some refactoring to approach it that way.  Moving that logic
>> from the platform-specific Makefiles into configure doesn't seem
>> unreasonable, but it would make the patch bigger.

> Sounds like a plan. I agree it's annoying to have to do something large
> for something so trivial.

Turns out it's not really that bad.  We just have to transfer the
responsibility for setting CFLAGS_SL from the platform Makefiles
to the platform template files.  (As a bonus, it'd be possible to
allow users to override CFLAGS_SL during configure, as they can
do for CFLAGS.  But I didn't mess with that here.)

I checked that this fixes the Fedora build problem, but I've not
really tested it on any other platform.  Still, there's not that
much to go wrong, one would think.

regards, tom lane

diff --git a/configure b/configure
index 02a905b..1d664a4 100755
--- a/configure
+++ b/configure
@@ -728,6 +728,7 @@ autodepend
 TAS
 GCC
 CPP
+CFLAGS_SL
 BITCODE_CXXFLAGS
 BITCODE_CFLAGS
 CFLAGS_VECTOR
@@ -6579,7 +6580,6 @@ fi
 
 fi
 
-CFLAGS_VECTOR=$CFLAGS_VECTOR
 
 
 # Determine flags used to emit bitcode for JIT inlining. Need to test
@@ -6899,9 +6899,10 @@ CXXFLAGS="$CXXFLAGS $user_CXXFLAGS"
 BITCODE_CFLAGS="$BITCODE_CFLAGS $user_BITCODE_CFLAGS"
 BITCODE_CXXFLAGS="$BITCODE_CXXFLAGS $user_BITCODE_CXXFLAGS"
 
-BITCODE_CFLAGS=$BITCODE_CFLAGS
 
-BITCODE_CXXFLAGS=$BITCODE_CXXFLAGS
+
+
+# The template file must set up CFLAGS_SL; we don't support user override
 
 
 # Check if the compiler still works with the final flag settings
diff --git a/configure.in b/configure.in
index 88d3a59..50f4b26 100644
--- a/configure.in
+++ b/configure.in
@@ -547,7 +547,7 @@ elif test "$PORTNAME" = "hpux"; then
   PGAC_PROG_CXX_CFLAGS_OPT([+Olibmerrno])
 fi
 
-AC_SUBST(CFLAGS_VECTOR, $CFLAGS_VECTOR)
+AC_SUBST(CFLAGS_VECTOR)
 
 # Determine flags used to emit bitcode for JIT inlining. Need to test
 # for behaviour changing compiler flags, to keep compatibility with
@@ -607,8 +607,11 @@ CXXFLAGS="$CXXFLAGS $user_CXXFLAGS"
 BITCODE_CFLAGS="$BITCODE_CFLAGS $user_BITCODE_CFLAGS"
 BITCODE_CXXFLAGS="$BITCODE_CXXFLAGS $user_BITCODE_CXXFLAGS"
 
-AC_SUBST(BITCODE_CFLAGS, $BITCODE_CFLAGS)
-AC_SUBST(BITCODE_CXXFLAGS, $BITCODE_CXXFLAGS)
+AC_SUBST(BITCODE_CFLAGS)
+AC_SUBST(BITCODE_CXXFLAGS)
+
+# The template file must set up CFLAGS_SL; we don't support user override
+AC_SUBST(CFLAGS_SL)
 
 # Check if the compiler still works with the final flag settings
 # (note, we're not checking that for CXX, which is optional)
diff --git a/src/Makefile.global.in b/src/Makefile.global.in
index 2d21068..05b6638 100644
--- a/src/Makefile.global.in
+++ b/src/Makefile.global.in
@@ -258,6 +258,7 @@ GCC = @GCC@
 SUN_STUDIO_CC = @SUN_STUDIO_CC@
 CXX = @CXX@
 CFLAGS = @CFLAGS@
+CFLAGS_SL = @CFLAGS_SL@
 CFLAGS_VECTOR = @CFLAGS_VECTOR@
 CFLAGS_SSE42 = @CFLAGS_SSE42@
 CFLAGS_ARMV8_CRC32C = @CFLAGS_ARMV8_CRC32C@
diff --git a/src/makefiles/Makefile.cygwin b/src/makefiles/Makefile.cygwin
index f274d80..81089d6 100644
--- a/src/makefiles/Makefile.cygwin
+++ b/src/makefiles/Makefile.cygwin
@@ -12,7 +12,6 @@ LIBS:=$(filter-out -lm -lc, $(LIBS))
 
 AROPT = crs
 DLSUFFIX = .dll
-CFLAGS_SL =
 
 override CPPFLAGS += -DWIN32_STACK_RLIMIT=$(WIN32_STACK_RLIMIT)
 
diff --git a/src/makefiles/Makefile.freebsd b/src/makefiles/Makefile.freebsd
index c462e2f..75db21b 100644
--- a/src/makefiles/Makefile.freebsd
+++ b/src/makefiles/Makefile.freebsd
@@ -5,8 +5,6 @@ rpath = -Wl,-R'$(rpathdir)'
 
 DLSUFFIX = .so
 
-CFLAGS_SL = -fPIC -DPIC
-
 # extra stuff for $(with_temp_install)
 # we need this to get LD_LIBRARY_PATH searched ahead of the compiled-in
 # rpath, if no DT_RUNPATH is present in the executable. The conditions
diff --git a/src/makefiles/Makefile.hpux b/src/makefiles/Makefile.hpux
index c871fb0..7e18770 100644
--- a/src/makefiles/Makefile.hpux
+++ b/src/makefiles/Makefile.hpux
@@ -30,11 +30,6 @@ ifeq ($(host_cpu), ia64)
 else
DLSUFFIX = .sl
 endif
-ifeq ($(GCC), yes)
-   CFLAGS_SL = -fPIC
-else
-   CFLAGS_SL = +Z
-endif
 
 # env var name to use in place of LD_LIBRARY_PATH
 ld_library_path_var = SHLIB_PATH
diff --git a/src/makefiles/Makefile.linux b/src/makefiles/Makefile.linux
index ac58fe4..645f73a 100644
--- a/src/makefiles/Makefile.linux
+++ b/src/makefiles/Makefile.linux
@@ -7,8 +7,6 @@ rpath = -Wl,-rpath,'$(rpathdir)',--enable-new-dtags
 
 DLSUFFIX = .so
 
-CFLAGS_SL = -fPIC
-
 
 # Rule for building a shared library from a single .o file
 %.so: %.o
diff --git a/src/makefiles/Makefile.netbsd b/src/makefiles/Makefile.netbsd
index 15695fb..6f9cb1d 100644
--- a/src/makefiles/Makefile.netbsd
+++ 

Re: jsonb_set() strictness considered harmful to data

[raf]

Steven Pousty wrote:

> I would think though that raising an exception is better than a
> default behavior which deletes data.

I can't help but feel the need to make the point that
the function is not deleting anything. It is just
returning null. The deletion of data is being performed
by an update statement that uses the function's return
value to set a column value.

I don't agree that raising an exception in the function
is a good idea (perhaps unless it's valid to assume
that this function will only ever be used in such a
context). Making the column not null (as already
suggested) and having the update statement itself raise
the exception seems more appropriate if an exception is
desirable. But that presumes an accurate understanding
of the behaviour of jsonb_set.

Really, I think the best fix would be in the
documentation so that everyone who finds the function
in the documentation understands its behaviour
immediately. I didn't even know there was such a thing
as a strict function or what it means and the
documentation for jsonb_set doesn't mention that it is
a strict function and the examples of its use don't
demonstrate this behaviour. I'm referring to
https://www.postgresql.org/docs/9.5/functions-json.html.

All of this contributes to the astonishment encountered
here. Least astonishment can probably be achieved with
additional documentation but it has to be where the
reader is looking when they first encounter the
function in the documentation so that their
expectations are set correctly and set early. And
documentation can be "fixed" sooner than postgresql 13.

Perhaps an audit of the documentation for all strict
functions would be a good idea to see if they need
work. Knowing that a function won't be executed at all
and will effectively return null when given a null
argument might be important to know for other functions
as well.

cheers,
raf

Re: Tweaking DSM and DSA limits

[Thomas Munro]

On Fri, Jun 21, 2019 at 6:52 AM Andres Freund  wrote:
> On 2019-06-20 14:20:27 -0400, Robert Haas wrote:
> > On Tue, Jun 18, 2019 at 9:08 PM Thomas Munro  wrote:
> > > Perhaps also the number of slots per backend should be dynamic, so
> > > that you have the option to increase it from the current hard-coded
> > > value of 2 if you don't want to increase max_connections but find
> > > yourself running out of slots (this GUC was a request from Andres but
> > > the name was made up by me -- if someone has a better suggestion I'm
> > > all ears).
> >
> > I am not convinced that we really need to GUC-ify this.  How about
> > just bumping the value up from 2 to say 5?
>
> I'm not sure either. Although it's not great if the only way out for a
> user hitting this is to increase max_connections... But we should really
> increase the default.

Ok, hard-to-explain GUC abandoned.  Here is a patch that just adjusts
the two constants.  DSM's array allows for 5 slots per connection (up
from 2), and DSA doubles its size after every two segments (down from
4).

> > As Andres observed off-list, it would also be a good idea to allow
> > things that are going to gobble memory like hash joins to have some
> > input into how much memory gets allocated.  Maybe preallocating the
> > expected size of the hash is too aggressive -- estimates can be wrong,
> > and it could be much smaller.
>
> At least for the case of the hashtable itself, we allocate that at the
> predicted size immediately. So a mis-estimation wouldn't change
> anything. For the entires, yea, something like you suggest would make
> sense.

At the moment the 32KB chunks are used as parallel granules for
various work (inserting, repartitioning, rebucketing).  I could
certainly allocate a much bigger piece based on estimates, and then
invent another kind of chunks inside that, or keep the existing
layering but find a way to hint to DSA what allocation stream to
expect in the future so it can get bigger underlying chunks ready.
One problem is that it'd result in large, odd sized memory segments,
whereas the current scheme uses power of two sizes and might be more
amenable to a later segment reuse scheme; or maybe that doesn't really
matter.

I have a long wish list of improvements I'd like to investigate in
this area, subject for future emails, but while I'm making small
tweaks, here's another small thing: there is no "wait event" while
allocating (in the kernel sense) POSIX shm on Linux, unlike the
equivalent IO when file-backed segments are filled with write() calls.
Let's just reuse the same wait event, so that you can see what's going
on in pg_stat_activity.


0001-Adjust-the-constants-used-to-reserve-DSM-segment-slo.patch
Description: Binary data


0002-Report-time-spent-in-posix_fallocate-as-a-wait-event.patch
Description: Binary data

Re: jsonb_set() strictness considered harmful to data

[Andrew Dunstan]

On 10/20/19 4:18 PM, Tomas Vondra wrote:
> On Sun, Oct 20, 2019 at 03:48:05PM -0400, Andrew Dunstan wrote:
>>
>> On 10/20/19 1:14 PM, David G. Johnston wrote:
>>> On Sun, Oct 20, 2019 at 5:31 AM Andrew Dunstan
>>> >> > wrote:
>>>
>>>     And yet another is to
>>>     raise an exception, which is easy to write but really punts the
>>> issue
>>>     back to the application programmer who will have to decide how to
>>>     ensure
>>>     they never pass in a NULL parameter.
>>>
>>>
>>> That's kinda the point - if they never pass NULL they won't encounter
>>> any problems but as soon as the data and their application don't see
>>> eye-to-eye the application developer has to decide what they want to
>>> do about it.  We are in no position to decide for them and making it
>>> obvious they have a decision to make and implement here doesn't seem
>>> like a improper position to take.
>>
>>
>> The app dev can avoid this problem today by making sure they don't pass
>> a NULL as the value. Or they can use a wrapper function which does that
>> for them. So frankly this doesn't seem like much of an advance. And, as
>> has been noted, it's not consistent with what either MySQL or MSSQL do.
>> In general I'm not that keen on raising an exception for cases like
>> this.
>>
>
> I think the general premise of this thread is that the application
> developer does not realize that may be necessary, because it's a bit
> surprising behavior, particularly when having more experience with other
> databases that behave differently. It's also pretty easy to not notice
> this issue for a long time, resulting in significant data loss.
>
> Let's say you're used to the MSSQL or MySQL behavior, you migrate your
> application to PostgreSQL or whatever - how do you find out about this
> behavior? Users are likely to visit
>
>    https://www.postgresql.org/docs/12/functions-json.html
>
> but that says nothing about how jsonb_set works with NULL values :-(



We should certainly fix that. I accept some responsibility for the omission.



>
> You're right raising an exception may not be the "right behavior" for
> whatever definition of "right". But I kinda agree with David that it's
> somewhat reasonable when we don't know what the "universally correct"
> thing is (or when there's no such thing). IMHO that's better than
> silently discarding some of the data.


I'm not arguing against the idea of improving the situation. But I am
arguing against a minimal fix that will not provide much of value to a
careful app developer. i.e. I want to do more to support app devs.
Ideally they would not need to use wrapper functions. There will be
plenty of situations where it is mighty inconvenient to catch an
exception thrown by jsonb_set(). And catching exceptions can be
expensive. You want to avoid that if possible in your
performance-critical plpgsql code.



>
> FWIW I think the JSON/JSONB part of our code base is amazing, and the
> fact that various other databases adopted something very similar over
> the last couple of years just confirms that. And if this is the only
> speck of dust in the API, I think that's pretty amazing.


TY. When I first saw the SQL/JSON spec I thought I should send a request
to the SQL standards committee for a royalty payment, since it looked so
familiar ;-)


>
> I'm not sure how significant this issue actually is - it's true we got a
> couple of complaints over the years (judging by a quick search for
> jsonb_set and NULL in the archives), but I'm not sure that's enough to
> justify any changes in backbranches. I'd say no, but I have no idea how
> many people are affected by this but don't know about it ...
>
>

No, no backpatching. As I said upthread, this isn't a bug, but it is
arguably a POLA violation, which is why we should do something for
release 13.


cheers


andrew


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

Re: Clean up MinGW def file generation

[Peter Eisentraut]

On 2019-10-20 10:26, Peter Eisentraut wrote:
> On 2019-10-18 15:00, Tom Lane wrote:
>> Yeah, the comment that Peter complained about is mine.  I believe the
>> desire to avoid depending on "sed" at build time was focused on our
>> old support for building libpq with Borland C (and not much else).
>> Since this makefile infrastructure is now only used for MinGW, I agree
>> we ought to be able to quit shipping those files in tarballs.
> 
> Yeah, it all makes sense now.  I have committed my patch now.

Very related, I believe the file libpq-dist.rc is also obsolete; see
attached patch.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From a098f5edc3477c774d06ddd9324364c2bad2c6fe Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Sun, 20 Oct 2019 23:57:57 +0200
Subject: [PATCH] Remove libpq-dist.rc

The use of this was removed by
6da56f3f84d430671d5edd8f9336bd744c089e31.
---
 src/interfaces/libpq/.gitignore | 1 -
 src/interfaces/libpq/Makefile   | 7 +--
 2 files changed, 1 insertion(+), 7 deletions(-)

diff --git a/src/interfaces/libpq/.gitignore b/src/interfaces/libpq/.gitignore
index 38779b23a4..9be338dec8 100644
--- a/src/interfaces/libpq/.gitignore
+++ b/src/interfaces/libpq/.gitignore
@@ -1,6 +1,5 @@
 /exports.list
 /libpq.rc
-/libpq-dist.rc
 # .c files that are symlinked in from elsewhere
 /encnames.c
 /wchar.c
diff --git a/src/interfaces/libpq/Makefile b/src/interfaces/libpq/Makefile
index 6626f87e76..2fad1bc44c 100644
--- a/src/interfaces/libpq/Makefile
+++ b/src/interfaces/libpq/Makefile
@@ -94,14 +94,10 @@ encnames.c wchar.c: % : $(backend_src)/utils/mb/%
rm -f $@ && $(LN_S) $< .
 
 
-distprep: libpq-dist.rc
-
-libpq.rc libpq-dist.rc: libpq.rc.in
+libpq.rc: libpq.rc.in
sed -e 's/\(VERSION.*\),0 *$$/\1,'`date '+%y%j' | sed 's/^0*//'`'/' $< 
>$@
 
 # Depend on Makefile.global to force rebuild on re-run of configure.
-# (But libpq-dist.rc is shipped in the distribution for shell-less
-# installations and is only updated by distprep.)
 libpq.rc: $(top_builddir)/src/Makefile.global
 
 # Make dependencies on pg_config_paths.h visible, too.
@@ -141,4 +137,3 @@ clean distclean: clean-lib
 
 maintainer-clean: distclean
$(MAKE) -C test $@
-   rm -f libpq-dist.rc
-- 
2.23.0

Re: jsonb_set() strictness considered harmful to data

[John W Higgins]

>
>
> You're not following because you don't want to follow.
>
>
I think that anyone with a "commit bit" on this project that tolerates that
sentence is a much better human being than I ever will be.

I may be the dumbest person on this list by many measures - but isn't there
standard options that are supposed to be the first line of defense here?
Why do I need a function to ensure I don't remove data by passing a NULL
value to an update?

Why would any of these standard statements not solve this issue?

update users set info=jsonb_set(info, '{bar}', info->'foo')
where info->'foo' is not null

update users set info=jsonb_set(info, '{bar}', info->'foo')
where jsonb_set(info, '{bar}', info->'foo') is not null

update users set info=coalesce(jsonb_set(info, '{bar}', info->'foo'), info)

I can totally respect the person that slams into this wall the first time
and is REALLY upset about it - but by their own admission this has occurred
multiple times in this project and they continue to not take standard
precautions.

Again, I applaud the patience of many people on this list. You deserve much
more respect than you are being shown here right now.

John W Higgins

Re: pause recovery if pitr target not reached

[Laurenz Albe]

On Sat, 2019-10-19 at 21:45 +0200, Peter Eisentraut wrote:
> On 2019-09-17 13:23, Leif Gunnar Erlandsen wrote:
> > This patch allows PostgreSQL to pause recovery before PITR target is 
> > reached 
> > if recovery_target_time is specified.
> > 
> > Missing WAL's could then be restored from backup and applied on next 
> > restart.
> > 
> > Today PostgreSQL opens the database in read/write on a new timeline even 
> > when 
> > PITR tareg is not reached.
> 
> I think this idea is worth thinking about.  I don't think this should be
> specific to a time-based recovery target.  This could apply for example
> to a target xid as well.  Also, there should be a way to get the old
> behavior.  Perhaps this whole thing should be a new
> recovery_target_action, say, 'pause_unless_reached'.

+1 for pausing if end-of-logs is reached before the recovery target.

I don't think that we need to add a new "recovery_target_action" to
retain the old behavior, because I think that nobody ever wants that.
I'd say that this typically happens in two cases:

1. Someone forgot to archive the WAL segment that contains the target.
   In this case the proposed change will solve the problem.

2. Someone specified the recovery target wrong, e.g. used CET rather
   than CEST in the recovery target time, so that the recovery target
   was later than intended.
   In that case the only solution is to start recovery from scratch.

But perhaps there are use cases I didn't think of.

Yours,
Laurenz Albe

Re: jsonb_set() strictness considered harmful to data

[Steven Pousty]

I would think though that raising an exception is better than a default
behavior which deletes data.
As an app dev I am quite used to all sorts of "APIs" throwing exceptions
and have learned to deal with them.

This is my way of saying that raising an exception is an improvement over
the current situation. May not be the "best" solution but definitely an
improvement.
Thanks
Steve

On Sun, Oct 20, 2019 at 12:48 PM Andrew Dunstan <
andrew.duns...@2ndquadrant.com> wrote:

>
> On 10/20/19 1:14 PM, David G. Johnston wrote:
> > On Sun, Oct 20, 2019 at 5:31 AM Andrew Dunstan
> >  > > wrote:
> >
> > And yet another is to
> > raise an exception, which is easy to write but really punts the issue
> > back to the application programmer who will have to decide how to
> > ensure
> > they never pass in a NULL parameter.
> >
> >
> > That's kinda the point - if they never pass NULL they won't encounter
> > any problems but as soon as the data and their application don't see
> > eye-to-eye the application developer has to decide what they want to
> > do about it.  We are in no position to decide for them and making it
> > obvious they have a decision to make and implement here doesn't seem
> > like a improper position to take.
>
>
> The app dev can avoid this problem today by making sure they don't pass
> a NULL as the value. Or they can use a wrapper function which does that
> for them. So frankly this doesn't seem like much of an advance. And, as
> has been noted, it's not consistent with what either MySQL or MSSQL do.
> In general I'm not that keen on raising an exception for cases like this.
>
>
> cheers
>
>
> andrew
>
>
> --
> Andrew Dunstanhttps://www.2ndQuadrant.com
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>
>
>
>

Re: jsonb_set() strictness considered harmful to data

[Tomas Vondra]

On Sun, Oct 20, 2019 at 03:48:05PM -0400, Andrew Dunstan wrote:


On 10/20/19 1:14 PM, David G. Johnston wrote:

On Sun, Oct 20, 2019 at 5:31 AM Andrew Dunstan
mailto:andrew.duns...@2ndquadrant.com>> wrote:

And yet another is to
raise an exception, which is easy to write but really punts the issue
back to the application programmer who will have to decide how to
ensure
they never pass in a NULL parameter.


That's kinda the point - if they never pass NULL they won't encounter
any problems but as soon as the data and their application don't see
eye-to-eye the application developer has to decide what they want to
do about it.  We are in no position to decide for them and making it
obvious they have a decision to make and implement here doesn't seem
like a improper position to take.



The app dev can avoid this problem today by making sure they don't pass
a NULL as the value. Or they can use a wrapper function which does that
for them. So frankly this doesn't seem like much of an advance. And, as
has been noted, it's not consistent with what either MySQL or MSSQL do.
In general I'm not that keen on raising an exception for cases like this.



I think the general premise of this thread is that the application
developer does not realize that may be necessary, because it's a bit
surprising behavior, particularly when having more experience with other
databases that behave differently. It's also pretty easy to not notice
this issue for a long time, resulting in significant data loss.

Let's say you're used to the MSSQL or MySQL behavior, you migrate your
application to PostgreSQL or whatever - how do you find out about this
behavior? Users are likely to visit

   https://www.postgresql.org/docs/12/functions-json.html

but that says nothing about how jsonb_set works with NULL values :-(

You're right raising an exception may not be the "right behavior" for
whatever definition of "right". But I kinda agree with David that it's
somewhat reasonable when we don't know what the "universally correct"
thing is (or when there's no such thing). IMHO that's better than
silently discarding some of the data.

FWIW I think the JSON/JSONB part of our code base is amazing, and the
fact that various other databases adopted something very similar over
the last couple of years just confirms that. And if this is the only
speck of dust in the API, I think that's pretty amazing.

I'm not sure how significant this issue actually is - it's true we got a
couple of complaints over the years (judging by a quick search for
jsonb_set and NULL in the archives), but I'm not sure that's enough to
justify any changes in backbranches. I'd say no, but I have no idea how
many people are affected by this but don't know about it ...

regards

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

Re: jsonb_set() strictness considered harmful to data

[Andrew Dunstan]

On 10/20/19 1:14 PM, David G. Johnston wrote:
> On Sun, Oct 20, 2019 at 5:31 AM Andrew Dunstan
>  > wrote:
>
> And yet another is to
> raise an exception, which is easy to write but really punts the issue
> back to the application programmer who will have to decide how to
> ensure
> they never pass in a NULL parameter.
>
>
> That's kinda the point - if they never pass NULL they won't encounter
> any problems but as soon as the data and their application don't see
> eye-to-eye the application developer has to decide what they want to
> do about it.  We are in no position to decide for them and making it
> obvious they have a decision to make and implement here doesn't seem
> like a improper position to take.


The app dev can avoid this problem today by making sure they don't pass
a NULL as the value. Or they can use a wrapper function which does that
for them. So frankly this doesn't seem like much of an advance. And, as
has been noted, it's not consistent with what either MySQL or MSSQL do.
In general I'm not that keen on raising an exception for cases like this.


cheers


andrew


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

Re: configure fails for perl check on CentOS8

[Andrew Dunstan]

On 10/20/19 1:23 PM, Tom Lane wrote:
> Andrew Dunstan  writes:
>> On 10/18/19 9:50 AM, Tom Lane wrote:
>>> Can we fix this by using something other than perl_alloc() as
>>> the tested-for function?  That is surely a pretty arbitrary
>>> choice.  Are there any standard Perl entry points that are just
>>> plain functions with no weird macro expansions?
>> I had a look in perl's proto.h but didn't see any obvious candidate. I
>> tried a couple of others (e.g. Perl_get_context) and got the same result
>> reported above.
> I poked into this on a Fedora 30 installation and determined that the
> stray reference is coming from this bit in Perl's inline.h:
>
> /* saves machine code for a common noreturn idiom typically used in Newx*() */
> GCC_DIAG_IGNORE_DECL(-Wunused-function);
> static void
> S_croak_memory_wrap(void)
> {
> Perl_croak_nocontext("%s",PL_memory_wrap);
> }
> GCC_DIAG_RESTORE_DECL;
>
> Apparently, gcc is smart enough to optimize this away as unused ...
> at any optimization level higher than -O0.  I confirmed that it works
> at -O0 too, if you change this function declaration to "static inline"
> --- but evidently, not doing so was intentional, so we won't get much
> cooperation if we propose changing it (back?) to a plain static inline.
>
> So the failure occurs just from reading this header, independently of
> which particular Perl function we try to call; I'd supposed that there
> was some connection between perl_alloc and PL_memory_wrap, but there
> isn't.



Yeah, I came to the same conclusion.


>
> I still don't much like the originally proposed patch.  IMO it makes too
> many assumptions about what is in Perl's ccflags, and perhaps more to the
> point, it is testing libperl's linkability using switches we will not use
> when we actually build plperl.  So I think there's a pretty high risk of
> breaking other cases if we fix it that way.


Agreed.


>
> The right way to fix it, likely, is to add CFLAGS_SL while performing this
> particular autoconf test, as that would replicate the switches used for
> plperl (and it turns out that adding -fPIC does solve this problem).
> But the configure script doesn't currently know about CFLAGS_SL, so we'd
> have to do some refactoring to approach it that way.  Moving that logic
> from the platform-specific Makefiles into configure doesn't seem
> unreasonable, but it would make the patch bigger.


Sounds like a plan. I agree it's annoying to have to do something large
for something so trivial.


>
> A less invasive idea is to forcibly add -O1 to CFLAGS for this autoconf
> test.  We'd have to be careful about doing so for non-gcc compilers, as
> they might not understand that switch syntax ... but probably we could
> get away with changing CFLAGS only when using a gcc-alike.  Still, that's
> a hack, and it doesn't have much to recommend it other than being more
> localized.
>
>   


right. I think your other plan is better.


cheers


andrew




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

Re: documentation inconsistent re: alignment

[Tom Lane]

Chapman Flack  writes:
> The documentation for CREATE TYPE has this to say about alignment:

> "The alignment parameter specifies the storage alignment required for the
> data type. The allowed values equate to alignment on 1, 2, 4, or 8 byte
> boundaries."

> ... while the documentation for pg_type has:

> "c = char alignment, i.e., no alignment needed.
> s = short alignment (2 bytes on most machines).
> i = int alignment (4 bytes on most machines).
> d = double alignment (8 bytes on many machines, but by no means all)."

> so, in 2019, are the alignments weaselly and variable, or are they 1,2,4,8?

Probably the statement in CREATE TYPE is too strong.  There are, I
believe, still machines in the buildfarm where maxalign is just 4.

regards, tom lane

Re: Ordering of header file inclusion

[vignesh C]

On Sun, Oct 20, 2019 at 2:44 AM Andres Freund  wrote:
>
> Hi,
>
> On 2019-10-19 21:50:03 +0200, Peter Eisentraut wrote:
> > diff --git a/contrib/bloom/blcost.c b/contrib/bloom/blcost.c
> > index f9fe57f..6224735 100644
> > --- a/contrib/bloom/blcost.c
> > +++ b/contrib/bloom/blcost.c
> > @@ -12,10 +12,10 @@
> >   */
> >  #include "postgres.h"
> >
> > +#include "bloom.h"
> >  #include "fmgr.h"
> >  #include "utils/selfuncs.h"
> >
> > -#include "bloom.h"
> >
> >  /*
> >   * Estimate cost of bloom index scan.
> >
> > This class of change I don't like.
> >
> > The existing arrangement keeps "other" header files separate from the
> > header file of the module itself.  It seems useful to keep that separate.
>
> If we were to do so, we ought to put bloom.h first and clearly seperated
> out, not last, as the former makes the bug of the the header not being
> standalone more obvious.
>
> I'm -1 on having a policy of putting the headers separate though, I feel
> that's too much work, and there's too many cases where it's not that
> clear which header that should be.
>
Thanks Andres for reviewing the changes, I have modified the changes
based on your suggestions.
I have included the module header file in the beginning.
The changes are attached in the previous mail.

Please let me know your suggestions for any changes.

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

Re: Ordering of header file inclusion

[vignesh C]

On Sun, Oct 20, 2019 at 1:20 AM Peter Eisentraut
 wrote:
>
> diff --git a/contrib/bloom/blcost.c b/contrib/bloom/blcost.c
> index f9fe57f..6224735 100644
> --- a/contrib/bloom/blcost.c
> +++ b/contrib/bloom/blcost.c
> @@ -12,10 +12,10 @@
>   */
>  #include "postgres.h"
>
> +#include "bloom.h"
>  #include "fmgr.h"
>  #include "utils/selfuncs.h"
>
> -#include "bloom.h"
>
>  /*
>   * Estimate cost of bloom index scan.
>
> This class of change I don't like.
>
> The existing arrangement keeps "other" header files separate from the
> header file of the module itself.  It seems useful to keep that separate.
>
Thanks Peter for your thoughts, I have modified the changes based on
your suggestions.
I have included the module header file in the beginning.
The changes are attached in the previous mail.

Please let me know your suggestions for any changes.

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

Re: configure fails for perl check on CentOS8

[Tom Lane]

Andrew Dunstan  writes:
> On 10/18/19 9:50 AM, Tom Lane wrote:
>> Can we fix this by using something other than perl_alloc() as
>> the tested-for function?  That is surely a pretty arbitrary
>> choice.  Are there any standard Perl entry points that are just
>> plain functions with no weird macro expansions?

> I had a look in perl's proto.h but didn't see any obvious candidate. I
> tried a couple of others (e.g. Perl_get_context) and got the same result
> reported above.

I poked into this on a Fedora 30 installation and determined that the
stray reference is coming from this bit in Perl's inline.h:

/* saves machine code for a common noreturn idiom typically used in Newx*() */
GCC_DIAG_IGNORE_DECL(-Wunused-function);
static void
S_croak_memory_wrap(void)
{
Perl_croak_nocontext("%s",PL_memory_wrap);
}
GCC_DIAG_RESTORE_DECL;

Apparently, gcc is smart enough to optimize this away as unused ...
at any optimization level higher than -O0.  I confirmed that it works
at -O0 too, if you change this function declaration to "static inline"
--- but evidently, not doing so was intentional, so we won't get much
cooperation if we propose changing it (back?) to a plain static inline.

So the failure occurs just from reading this header, independently of
which particular Perl function we try to call; I'd supposed that there
was some connection between perl_alloc and PL_memory_wrap, but there
isn't.

I still don't much like the originally proposed patch.  IMO it makes too
many assumptions about what is in Perl's ccflags, and perhaps more to the
point, it is testing libperl's linkability using switches we will not use
when we actually build plperl.  So I think there's a pretty high risk of
breaking other cases if we fix it that way.

The right way to fix it, likely, is to add CFLAGS_SL while performing this
particular autoconf test, as that would replicate the switches used for
plperl (and it turns out that adding -fPIC does solve this problem).
But the configure script doesn't currently know about CFLAGS_SL, so we'd
have to do some refactoring to approach it that way.  Moving that logic
from the platform-specific Makefiles into configure doesn't seem
unreasonable, but it would make the patch bigger.

A less invasive idea is to forcibly add -O1 to CFLAGS for this autoconf
test.  We'd have to be careful about doing so for non-gcc compilers, as
they might not understand that switch syntax ... but probably we could
get away with changing CFLAGS only when using a gcc-alike.  Still, that's
a hack, and it doesn't have much to recommend it other than being more
localized.

regards, tom lane

Re: jsonb_set() strictness considered harmful to data

[David G. Johnston]

On Sun, Oct 20, 2019 at 5:31 AM Andrew Dunstan <
andrew.duns...@2ndquadrant.com> wrote:

> And yet another is to
> raise an exception, which is easy to write but really punts the issue
> back to the application programmer who will have to decide how to ensure
> they never pass in a NULL parameter.


That's kinda the point - if they never pass NULL they won't encounter any
problems but as soon as the data and their application don't see eye-to-eye
the application developer has to decide what they want to do about it.  We
are in no position to decide for them and making it obvious they have a
decision to make and implement here doesn't seem like a improper position
to take.


> Possibly we could even add an extra
> parameter to specify what should be done.
>

Has appeal.



> Should we return NULL in those cases as we do now?
>

Probably the same thing - though I'd accept having the input json being
null result in the output json being null as well.

David J.

Re: Declaring a strict function returns not null / eval speed

[Tels]

Moin,

On 2019-10-20 16:27, Tom Lane wrote:

Tels  writes:

On 2019-10-20 13:30, Andreas Karlsson wrote:

Agreed, this sounds like something useful to do since virtually all
strict functions cannot return NULL, especially the ones which are
used in tight loops. The main design issue seems to be to think up a
name for this new level of strictness which is not too confusing for
end users.



STRICT NONULL? That way you could do



   CREATE FUNCTION f1 ... STRICT;
   CREATE FUNCTION f2 ... STRICT NONULL;
   CREATE FUNCTION f3 ... NONULL;



and the last wold throw "not implementet yet"? "NEVER RETURNS NULL"
would also ryme with the existing "RETURNS NULL ON NULL INPUT", but I
find the verbosity too high.


"RETURNS NOT NULL", perhaps?  That'd have the advantage of not 
requiring

any new keyword.


Hm, yes, that would be a good compromise on verbosity and align even 
better the other "RETURNS ..." variants.


I'm a little bit skeptical of the actual value of adding this 
additional

level of complexity, but I suppose we can't determine that reliably
without doing most of the work :-(


Maybe it would be possible to simulate the effect somehow? Or at least 
we could try to find practical queries where the additional information 
results in a much better plan if RETRUNS NOT NULL was set.


Best regards,

Tels

Re: Declaring a strict function returns not null / eval speed

[Tom Lane]

Tels  writes:
> On 2019-10-20 13:30, Andreas Karlsson wrote:
>> Agreed, this sounds like something useful to do since virtually all
>> strict functions cannot return NULL, especially the ones which are
>> used in tight loops. The main design issue seems to be to think up a
>> name for this new level of strictness which is not too confusing for
>> end users.

> STRICT NONULL? That way you could do

>CREATE FUNCTION f1 ... STRICT;
>CREATE FUNCTION f2 ... STRICT NONULL;
>CREATE FUNCTION f3 ... NONULL;

> and the last wold throw "not implementet yet"? "NEVER RETURNS NULL" 
> would also ryme with the existing "RETURNS NULL ON NULL INPUT", but I 
> find the verbosity too high.

"RETURNS NOT NULL", perhaps?  That'd have the advantage of not requiring
any new keyword.

I'm a little bit skeptical of the actual value of adding this additional
level of complexity, but I suppose we can't determine that reliably
without doing most of the work :-(

regards, tom lane

Re: jsonb_set() strictness considered harmful to data

[Isaac Morland]

On Sun, 20 Oct 2019 at 08:32, Andrew Dunstan 
wrote:

>
> Understood. I think the real question here is what it should do instead
> when the value is NULL. Your behaviour above is one suggestion, which I
> personally find intuitive. Another has been to remove the associated
> key. Another is to return the original target. And yet another is to
> raise an exception, which is easy to write but really punts the issue
> back to the application programmer who will have to decide how to ensure
> they never pass in a NULL parameter. Possibly we could even add an extra
> parameter to specify what should be done.
>

I vote for remove the key. If we make NULL and 'null'::jsonb the same,
we're missing an opportunity to provide more functionality. Sometimes it's
convenient to be able to handle both the "update" and "remove" cases with
one function, just depending on the parameter value supplied.

Also, the question will arise what to do when any of the other
> parameters are NULL. Should we return NULL in those cases as we do now?
>

I would argue that only if the target parameter (the actual json value) is
NULL should the result be NULL. The function is documented as returning the
target, with modifications to a small part of its structure as specified by
the other parameters. It is strange for the result to suddenly collapse
down to NULL just because another parameter is NULL. Perhaps if the path is
NULL, that can mean "don't update". And if create_missing is NULL, that
should mean the same as not specifying it. I think. At a minimum, if we
don't change it, the documentation needs to get one of those warning boxes
alerting people that the functions will destroy their input entirely rather
than slightly modifying it if any of the other parameters are NULL.

My only doubt about any of this is that by the same argument, functions
like replace() should not return NULL if the 2nd or 3rd parameter is NULL.
I'm guessing replace() is specified by SQL and also unchanged in many
versions so therefore not eligible for re-thinking but it still gives me
just a bit of pause.

Re: jsonb_set() strictness considered harmful to data

[Andrew Dunstan]

On 10/20/19 4:39 AM, Floris Van Nee wrote:
>
> FWIW I've been bitten by this 'feature' more than once as well,
> accidentally erasing a column. Now I usually write js = jsonb_set(js,
> coalesce(new_column, 'null'::jsonb)) to prevent erasing the whole
> column, and instead setting the value to a jsonb null value, but I
> also found the STRICT behavior very surprising at first..
>
>
>



Understood. I think the real question here is what it should do instead
when the value is NULL. Your behaviour above is one suggestion, which I
personally find intuitive. Another has been to remove the associated
key. Another is to return the original target. And yet another is to
raise an exception, which is easy to write but really punts the issue
back to the application programmer who will have to decide how to ensure
they never pass in a NULL parameter. Possibly we could even add an extra
parameter to specify what should be done.


Also, the question will arise what to do when any of the other
parameters are NULL. Should we return NULL in those cases as we do now?


cheers


andrew


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

Re: Declaring a strict function returns not null / eval speed

[Tels]

Moin,

On 2019-10-20 13:30, Andreas Karlsson wrote:

On 10/1/19 9:38 AM, Andres Freund wrote:
We spend a surprising amount of time during expression evaluation to 
reevaluate whether input to a strict function (or similar) is not 
null, even though the value either comes from a strict function, or a 
column declared not null.


Now you can rightfully say that a strict function still can return 
NULL, even when called with non-NULL input. But practically that's 
quite rare. Most of the common byvalue type operators are strict, and 
approximately none of those return NULL when actually called.


That makes me wonder if it's worthwhile to invent a function property 
declaring strict strictness or such. It'd allow for some quite 
noticable improvements for e.g. queries aggregating a lot of rows, we 
spend a fair time checking whether the transition value has "turned" 
not null. I'm about to submit a patch making that less expensive, but 
it's still expensive.


I can also imagine that being able to propagate NOT NULL further up 
the parse-analysis tree could be beneficial for planning, but I've not 
looked at it in any detail.


Agreed, this sounds like something useful to do since virtually all
strict functions cannot return NULL, especially the ones which are
used in tight loops. The main design issue seems to be to think up a
name for this new level of strictness which is not too confusing for
end users.


STRICT NONULL? That way you could do

  CREATE FUNCTION f1 ... STRICT;
  CREATE FUNCTION f2 ... STRICT NONULL;
  CREATE FUNCTION f3 ... NONULL;

and the last wold throw "not implementet yet"? "NEVER RETURNS NULL" 
would also ryme with the existing "RETURNS NULL ON NULL INPUT", but I 
find the verbosity too high.


Best regards,

Tels

--
Best regards,

Tels

Re: Declaring a strict function returns not null / eval speed

[Andreas Karlsson]

On 10/1/19 9:38 AM, Andres Freund wrote:
We spend a surprising amount of time during expression evaluation to 
reevaluate whether input to a strict function (or similar) is not null, 
even though the value either comes from a strict function, or a column 
declared not null.


Now you can rightfully say that a strict function still can return NULL, 
even when called with non-NULL input. But practically that's quite rare. 
Most of the common byvalue type operators are strict, and approximately 
none of those return NULL when actually called.


That makes me wonder if it's worthwhile to invent a function property 
declaring strict strictness or such. It'd allow for some quite noticable 
improvements for e.g. queries aggregating a lot of rows, we spend a fair 
time checking whether the transition value has "turned" not null. I'm 
about to submit a patch making that less expensive, but it's still 
expensive.


I can also imagine that being able to propagate NOT NULL further up the 
parse-analysis tree could be beneficial for planning, but I've not 
looked at it in any detail.


Agreed, this sounds like something useful to do since virtually all 
strict functions cannot return NULL, especially the ones which are used 
in tight loops. The main design issue seems to be to think up a name for 
this new level of strictness which is not too confusing for end users.


We also have a handful of non-strict functions (e.g. concat() and range 
constructors like tstzrange()) which are guaranteed to never return 
NULL, but I do not think they are many enough or performance critical 
enough to be worth adding this optimization to.


Andreas

jsonb_set() strictness considered harmful to data

[Floris Van Nee]

FWIW I've been bitten by this 'feature' more than once as well, accidentally 
erasing a column. Now I usually write js = jsonb_set(js, coalesce(new_column, 
'null'::jsonb)) to prevent erasing the whole column, and instead setting the 
value to a jsonb null value, but I also found the STRICT behavior very 
surprising at first..


-Floris

Re: Clean up MinGW def file generation

[Peter Eisentraut]

On 2019-10-18 15:00, Tom Lane wrote:
> Yeah, the comment that Peter complained about is mine.  I believe the
> desire to avoid depending on "sed" at build time was focused on our
> old support for building libpq with Borland C (and not much else).
> Since this makefile infrastructure is now only used for MinGW, I agree
> we ought to be able to quit shipping those files in tarballs.

Yeah, it all makes sense now.  I have committed my patch now.

> I think there could be some .gitignore cleanup done along with this.
> Notably, I see exclusions for /exports.list in several places, but no
> other references to that name --- isn't that an intermediate file that
> we used to generate while creating these files?

exports.list is built from exports.txt on non-Windows platforms and
AFAICT it is not cleaned up as an intermediate file.  So I think the
current arrangement is correct.

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

Re: Remove obsolete information schema tables

[Peter Eisentraut]

On 2019-10-17 09:44, Michael Paquier wrote:
> I have a question here.  Per the notes in information_schema.sql,
> SQL_SIZING_PROFILES has been removed in SQL:2011,

OK, we can remove that one as well.  New patch attached.

> attributes.isnullable and DOMAIN_UDT_USAGE in SQL:2003~.  Would it
> make sense to cleanup those ones?

OK, I'll look into those, but it seems like a separate undertaking.  We
don't always remove things just because they were dropped by the SQL
standard.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>From aec53faf22966ee56ccd812996e40527cc2cea49 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Sat, 19 Oct 2019 22:56:18 +0200
Subject: [PATCH v2] Remove obsolete information schema tables

Remove SQL_LANGUAGES, which was eliminated in SQL:2008, and
SQL_PACKAGES and SQL_SIZING_PROFILES, which were eliminated in
SQL:2011.  Since they were dropped by the SQL standard, the
information in them was no longer updated and therefore no longer
useful.

This also removes the feature-package association information in
sql_feature_packages.txt, but for the time begin we are keeping the
information which features are in the Core package (that is, mandatory
SQL features).  Maybe at some point someone wants to invent a way to
store that that does not involve using the "package" mechanism
anymore.

Discussion 
https://www.postgresql.org/message-id/flat/91334220-7900-071b-9327-0c6ecd012017%402ndquadrant.com
---
 doc/src/sgml/features.sgml   |   9 +-
 doc/src/sgml/information_schema.sgml | 216 ---
 src/backend/catalog/information_schema.sql   |  70 --
 src/backend/catalog/sql_feature_packages.txt |  29 ---
 src/backend/catalog/sql_features.txt |   4 -
 src/test/regress/expected/sanity_check.out   |   3 -
 6 files changed, 3 insertions(+), 328 deletions(-)

diff --git a/doc/src/sgml/features.sgml b/doc/src/sgml/features.sgml
index f767bee46e..2c5a7e5d0c 100644
--- a/doc/src/sgml/features.sgml
+++ b/doc/src/sgml/features.sgml
@@ -44,10 +44,7 @@ SQL Conformance
   broad three levels found in SQL-92.  A large
   subset of these features represents the Core
   features, which every conforming SQL implementation must supply.
-  The rest of the features are purely optional.  Some optional
-  features are grouped together to form packages, which
-  SQL implementations can claim conformance to, thus claiming
-  conformance to particular groups of features.
+  The rest of the features are purely optional.
  
 
  
@@ -116,7 +113,7 @@ Supported Features
   

 Identifier
-Package
+Core?
 Description
 Comment

@@ -143,7 +140,7 @@ Unsupported Features
   

 Identifier
-Package
+Core?
 Description
 Comment

diff --git a/doc/src/sgml/information_schema.sgml 
b/doc/src/sgml/information_schema.sgml
index 906fe7819f..7a995a1b64 100644
--- a/doc/src/sgml/information_schema.sgml
+++ b/doc/src/sgml/information_schema.sgml
@@ -4963,160 +4963,6 @@ sql_implementation_info 
Columns
   
  
 
- 
-  sql_languages
-
-  
-   The table sql_languages contains one row for
-   each SQL language binding that is supported by
-   PostgreSQL.
-   PostgreSQL supports direct SQL and
-   embedded SQL in C; that is all you will learn from this table.
-  
-
-  
-   This table was removed from the SQL standard in SQL:2008, so there
-   are no entries referring to standards later than SQL:2003.
-  
-
-  
-   sql_languages Columns
-
-   
-
- 
-  Name
-  Data Type
-  Description
- 
-
-
-
- 
-  sql_language_source
-  character_data
-  
-   The name of the source of the language definition; always
-   ISO 9075, that is, the SQL standard
-  
- 
-
- 
-  sql_language_year
-  character_data
-  
-   The year the standard referenced in
-   sql_language_source was approved.
-  
- 
-
- 
-  sql_language_conformance
-  character_data
-  
-   The standard conformance level for the language binding.  For
-   ISO 9075:2003 this is always CORE.
-  
- 
-
- 
-  sql_language_integrity
-  character_data
-  Always null (This value is relevant to an earlier version of the 
SQL standard.)
- 
-
- 
-  sql_language_implementation
-  character_data
-  Always null
- 
-
- 
-  sql_language_binding_style
-  character_data
-  
-   The language binding style, either DIRECT or
-   EMBEDDED
-  
- 
-
- 
-  sql_language_programming_language
-  character_data
-  
-   The programming language, if the binding style is
-   EMBEDDED, else null.  
PostgreSQL only
-   supports the language C.
-  
- 
-
-   
-  
- 
-
- 
-  sql_packages
-
-  
-   The table sql_packages contains information
-   about which