Re: assert pg_class.relnatts is consistent

[Amit Langote]

On Thu, Feb 13, 2020 at 3:23 AM Justin Pryzby  wrote:
> Forking this thread for two tangential patches which I think are more
> worthwhile than the original topic's patch.
> https://www.postgresql.org/message-id/20200207143935.GP403%40telsasoft.com
>
> Is there a better place to implement assertion from 0002 ?

I would think the answer to that would be related to the answer of why
you think we need this assert in the first place?

I know I have made the mistake of not updating relnatts when I added
relispartition, etc. to pg_class, only to be bitten by it in the form
of seemingly random errors/crashes.  Is that why?

Thanks,
Amit

Re: Error on failed COMMIT

[Haumacher, Bernhard]

Am 12.02.2020 um 00:27 schrieb Tom Lane:

Vik Fearing  writes:

Actually, I was imagining that it would end the transaction as it does
today, just with an error code.
This is backed up by General Rule 9 which says "The current
SQL-transaction is terminated."

Hm ... that would be sensible, but I'm not entirely convinced.  There
are several preceding rules that say that an exception condition is
raised, and normally you can stop reading at that point; nothing else
is going to happen.  If COMMIT acts specially in this respect, they
ought to say so.

In any case, while this interpretation might change the calculus a bit,
I think we still end up concluding that altering this behavior has more
downside than upside.


Let me illustrate this issue from an application (framework) developer's 
perspective:


When an application interacts with a database, it must be clearly 
possible to determine, whether a commit actually succeeded (and made all 
changes persistent), or the commit failed for any reason (and all of the 
changes have been rolled back). If a commit succeeds, an application 
must be allowed to assume that all changes it made in the preceeding 
transaction are made persistent and it is valid to update its internal 
state (e.g. caches) to the values updated in the transaction. This must 
be possible, even if the transaction is constructed collaboratively by 
multipe independent layers of the application (e.g. a framework and an 
application layer). Unfortunately, this seems not to be possible with 
the current implementation - at least not with default settings:


Assume the application is written in Java and sees Postgres through the 
JDBC driver:


composeTransaction() {
   Connection con = getConnection(); // implicitly "begin"
   try {
  insertFrameworkLevelState(con);
  insertApplicationLevelState(con);
  con.commit();
  publishNewState();
   } catch (Throwable ex) {
  con.rollback();
   }
}

With the current implementation, it is possible, that the control flow 
reaches "publishNewState()" without the changes done in 
"insertFrameworkLevelState()" have been made persistent - without the 
framework-level code (which is everything except 
"insertApplicationLevelState()") being able to detect the problem (e.g. 
if "insertApplicationLevelState()" tries add a null into a non-null 
column catching the exception or any other application-level error that 
is not properly handled through safepoints).


From a framework's perspective, this behavior is absolutely 
unacceptable. Here, the framework-level code sees a database that 
commits successfully but does not make its changes persistent. 
Therefore, I don't think that altering this behavior has more downside 
than upside.


Best regards

Bernhard

Re: Identifying user-created objects

[Amit Langote]

On Thu, Feb 13, 2020 at 10:30 AM Kyotaro Horiguchi
 wrote:
> At Mon, 10 Feb 2020 14:32:44 +0900, Amit Langote  
> wrote in
> > Agree that ObjectIsUserObject(oid) is easier to read than oid >=
> > FirstNormalObject.  I would have not bothered, for example, if it was
> > something like oid >= FirstUserObjectId to begin with.
>
> Aside from the naming, I'm not sure it's sensible to use
> FirstNormalObjectId since I don't see a clear definition or required
> characteristics for "user created objects" is.  If we did CREATE
> TABLE, FUNCTION or maybe any objects during single-user mode before
> the first object is created during normal multiuser operation, the
> "user-created(or not?)" object has an OID less than
> FirstNormalObjectId. If such objects are the "user created object", we
> need FirstUserObjectId defferent from FirstNormalObjectId.

Interesting observation.  Connecting to database in --single mode,
whether done using initdb or directly, is always considered
"bootstrapping", so the OIDs from the bootstrapping range are
consumed.

$ postgres --single -D pgdata postgres

PostgreSQL stand-alone backend 13devel
backend> create table a (a int);
backend> select 'a'::regclass::oid;
 1: oid (typeid = 26, len = 4, typmod = -1, byval = t)

 1: oid = "14168"   (typeid = 26, len = 4, typmod = -1, byval = t)

Here, FirstBootstrapObjectId < 14168 < FirstNormalObjectId

Maybe we could document that pg_is_user_object() and its internal
counterpart returns true only for objects that are created during
"normal" multi-user database operation.

Thanks,
Amit

Re: Wait event that should be reported while waiting for WAL archiving to finish

[Michael Paquier]

On Thu, Feb 13, 2020 at 03:35:50PM +0900, Fujii Masao wrote:
> I found that the wait events "LogicalRewriteTruncate" and
> "GSSOpenServer" are not documented. I'm thinking to add
> them into doc separately if ok.

Nice catch.  The ordering of the entries is not respected either for
GSSOpenServer in pgstat.h.  The portion for the code and the docs can
be fixed in back-branches, but not the enum list in WaitEventClient or
we would have an ABI breakage.  But this can be fixed on HEAD.  Can
you take care of it?  If you need help, please feel free to poke me. I
think that this should be fixed first, before adding the new event.

>   SyncRep
>   Waiting for confirmation from remote server during 
> synchronous replication.
>  
> +
> + BackupWaitWalArchive
> + Waiting for WAL files required for the backup to be 
> successfully archived.
> +

The category IPC is adapted.  You forgot to update the markup morerows
from "36" to "37", causing the table of the wait events to have a
weird format (the bottom should be incorrect).

> + pgstat_report_wait_start(WAIT_EVENT_BACKUP_WAIT_WAL_ARCHIVE);
>   while (XLogArchiveIsBusy(lastxlogfilename) ||
>  XLogArchiveIsBusy(histfilename))
>   {
> @@ -11120,6 +11121,7 @@ do_pg_stop_backup(char *labelfile, bool 
> waitforarchive, TimeLineID *stoptli_p)
>"but the 
> database backup will not be usable without all the WAL segments.")));
>   }
>   }
> + pgstat_report_wait_end();

Okay, that position is right.

> @@ -3848,6 +3848,9 @@ pgstat_get_wait_ipc(WaitEventIPC w)
>   case WAIT_EVENT_SYNC_REP:
>   event_name = "SyncRep";
>   break;
> + case WAIT_EVENT_BACKUP_WAIT_WAL_ARCHIVE:
> + event_name = "BackupWaitWalArchive";
> + break;
>   /* no default case, so that compiler will warn */
> [...]
> @@ -853,7 +853,8 @@ typedef enum
>   WAIT_EVENT_REPLICATION_ORIGIN_DROP,
>   WAIT_EVENT_REPLICATION_SLOT_DROP,
>   WAIT_EVENT_SAFE_SNAPSHOT,
> - WAIT_EVENT_SYNC_REP
> + WAIT_EVENT_SYNC_REP,
> + WAIT_EVENT_BACKUP_WAIT_WAL_ARCHIVE
>  } WaitEventIPC;

It would be good to keep entries in alphabetical order in the header,
the code and in the docs (see the effort from 5ef037c), and your patch
is missing that concept for all three places where it matters for this
new event.
--
Michael


signature.asc
Description: PGP signature

Re: Getting rid of some more lseek() calls

[Michael Paquier]

On Thu, Feb 13, 2020 at 02:51:44PM +1300, Thomas Munro wrote:
> Ok, how about this?

Alvaro's point sounds sensible to me.  I like the approach you are
taking in 0001.  At least it avoids more issues with WIN32 and stat()
(I hope to work on that at some point, we'll see..).

+/*
+ * pg_file_size --- return the size of a file
+ */
+int64
+pg_file_size(int fd)
+{
This routine has nothing really dependent on the backend.  Would it
make sense to put it in a different place where it can be used by the
frontend?  The function should include at least a comment about why we
have a special path for Windows, aka not falling into the trap of the
4GB limit for stat().

The commit message of 0001 mentions pg_read(), and that should be
pg_pread().

There are two combinations of lseek/read that could be replaced: one
in pg_receivewal.c:FindStreamingStart(), and one in
SimpleXLogPageRead() for parsexlog.c as of pg_rewind.

Patch 0002 looks good to me.  This actually removes a confusion when
failing to seek the end of the file as the offset referenced to would
be 0.  Patch 0003 is also a very good thing.
--
Michael


signature.asc
Description: PGP signature

Re: Wait event that should be reported while waiting for WAL archiving to finish

[Fujii Masao]

On 2020/02/13 12:28, Michael Paquier wrote:

On Thu, Feb 13, 2020 at 02:29:20AM +0900, Fujii Masao wrote:

When I saw pg_stat_activity.wait_event while pg_basebackup -X none
is waiting for WAL archiving to finish, it was either NULL or
CheckpointDone. I think this is confusing. What about introducing
new wait_event like WAIT_EVENT_BACKUP_WAIT_WAL_ARCHIVE
(BackupWaitWalArchive) and reporting it during that period?


Sounds like a good idea to me.  You need to be careful that this does
not overwrite more low-level wait event registration though, so that
could be more tricky than it looks at first sight.


Thanks for the advise! Patch attached.

I found that the wait events "LogicalRewriteTruncate" and
"GSSOpenServer" are not documented. I'm thinking to add
them into doc separately if ok.

Regards,

--
Fujii Masao
NTT DATA CORPORATION
Advanced Platform Technology Group
Research and Development Headquarters
diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index 9129f79bbf..1e2f81d994 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -1479,6 +1479,10 @@ postgres   27093  0.0  0.0  30096  2752 ?Ss   
11:34   0:00 postgres: ser
  SyncRep
  Waiting for confirmation from remote server during synchronous 
replication.
 
+
+ BackupWaitWalArchive
+ Waiting for WAL files required for the backup to be 
successfully archived.
+
 
  Timeout
  BaseBackupThrottle
diff --git a/src/backend/access/transam/xlog.c 
b/src/backend/access/transam/xlog.c
index 3813eadfb4..d79417b443 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -11095,6 +11095,7 @@ do_pg_stop_backup(char *labelfile, bool waitforarchive, 
TimeLineID *stoptli_p)
seconds_before_warning = 60;
waits = 0;
 
+   pgstat_report_wait_start(WAIT_EVENT_BACKUP_WAIT_WAL_ARCHIVE);
while (XLogArchiveIsBusy(lastxlogfilename) ||
   XLogArchiveIsBusy(histfilename))
{
@@ -11120,6 +11121,7 @@ do_pg_stop_backup(char *labelfile, bool waitforarchive, 
TimeLineID *stoptli_p)
 "but the 
database backup will not be usable without all the WAL segments.")));
}
}
+   pgstat_report_wait_end();
 
ereport(NOTICE,
(errmsg("all required WAL segments have been 
archived")));
diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
index 7169509a79..6114ab44ee 100644
--- a/src/backend/postmaster/pgstat.c
+++ b/src/backend/postmaster/pgstat.c
@@ -3848,6 +3848,9 @@ pgstat_get_wait_ipc(WaitEventIPC w)
case WAIT_EVENT_SYNC_REP:
event_name = "SyncRep";
break;
+   case WAIT_EVENT_BACKUP_WAIT_WAL_ARCHIVE:
+   event_name = "BackupWaitWalArchive";
+   break;
/* no default case, so that compiler will warn */
}
 
diff --git a/src/include/pgstat.h b/src/include/pgstat.h
index aecb6013f0..7c40ebc3dc 100644
--- a/src/include/pgstat.h
+++ b/src/include/pgstat.h
@@ -853,7 +853,8 @@ typedef enum
WAIT_EVENT_REPLICATION_ORIGIN_DROP,
WAIT_EVENT_REPLICATION_SLOT_DROP,
WAIT_EVENT_SAFE_SNAPSHOT,
-   WAIT_EVENT_SYNC_REP
+   WAIT_EVENT_SYNC_REP,
+   WAIT_EVENT_BACKUP_WAIT_WAL_ARCHIVE
 } WaitEventIPC;
 
 /* --

Re: Cache relation sizes?

[Thomas Munro]

On Tue, Feb 4, 2020 at 2:23 AM Andres Freund  wrote:
> On 2019-12-31 17:05:31 +1300, Thomas Munro wrote:
> > There is one potentially interesting case that doesn't require any
> > kind of shared cache invalidation AFAICS.  XLogReadBufferExtended()
> > calls smgrnblocks() for every buffer access, even if the buffer is
> > already in our buffer pool.
>
> Yea, that's really quite bad*. The bit about doing so even when already
> in the buffer pool is particularly absurd.  Needing to have special
> handling in mdcreate() for XLogReadBufferExtended() always calling it is
> also fairly ugly.

Yeah.  It seems like there are several things to fix there.  So now
I'm wondering if we should start out by trying to cache the size it in
the smgr layer for recovery only, like in the attached, and then later
try to extend the scheme to cover the shared case as discussed at the
beginning of the thread.

> A word of caution about strace's -c: In my experience the total time
> measurements are very imprecise somehow. I think it might be that some
> of the overhead of ptracing will be attributed to the syscalls or such,
> which means frequent syscalls appear relatively more expensive than they
> really are.

Yeah, those times are large, meaningless tracing overheads.  While
some systems might in fact be happy replaying a couple of million
lseeks per second, (1) I'm pretty sure some systems would not be happy
about that (see claims in this thread) and (2) it means you can't
practically use strace on recovery because it slows it down to a
crawl, which is annoying.


0001-Use-cached-smgrnblocks-results-in-recovery.patch
Description: Binary data

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

[Mahendra Singh Thalor]

On Thu, 13 Feb 2020 at 09:46, Amit Kapila  wrote:
>
> On Tue, Feb 11, 2020 at 9:13 PM Tom Lane  wrote:
> >
> > I took a brief look through this patch.  I agree with the fundamental
> > idea that we shouldn't need to use the heavyweight lock manager for
> > relation extension, since deadlock is not a concern and no backend
> > should ever need to hold more than one such lock at once.  But it feels
> > to me like this particular solution is rather seriously overengineered.
> > I would like to suggest that we do something similar to Robert Haas'
> > excellent hack (daa7527af) for the !HAVE_SPINLOCK case in lmgr/spin.c,
> > that is,
> >
> > * Create some predetermined number N of LWLocks for relation extension.
> > * When we want to extend some relation R, choose one of those locks
> >   (say, R's relfilenode number mod N) and lock it.
> >
>
> I am imagining something on the lines of BufferIOLWLockArray (here it
> will be RelExtLWLockArray).  The size (N) could MaxBackends or some
> percentage of it (depending on testing) and indexing into an array
> could be as suggested (R's relfilenode number mod N).  We need to
> initialize this during shared memory initialization.  Then, to extend
> the relation with multiple blocks at-a-time (as we do in
> RelationAddExtraBlocks), we can either use the already proven
> technique of group clear xid mechanism (see ProcArrayGroupClearXid) or
> have an additional state in the RelExtLWLockArray which will keep the
> count of waiters (as done in latest patch of Sawada-san [1]).  We
> might want to experiment with both approaches and see which yields
> better results.

Thanks all for the suggestions. I have started working on the
implementation based on the suggestion.  I will post a patch for this
in few days.

-- 
Thanks and Regards
Mahendra Singh Thalor
EnterpriseDB: http://www.enterprisedb.com

Re: Implementing Incremental View Maintenance

[Yugo NAGATA]

Hi PAscal,

On Tue, 11 Feb 2020 15:04:12 -0700 (MST)
legrand legrand  wrote:
> 
> regarding syntax REFRESH MATERIALIZED VIEW x WITH NO DATA
> 
> I understand that triggers are removed from the source tables, transforming 
> the INCREMENTAL MATERIALIZED VIEW into a(n unscannable) MATERIALIZED VIEW.
> 
> postgres=# refresh materialized view imv with no data;
> REFRESH MATERIALIZED VIEW
> postgres=# select * from imv;
> ERROR:  materialized view "imv" has not been populated
> HINT:  Use the REFRESH MATERIALIZED VIEW command.
> 
> This operation seems to me more of an ALTER command than a REFRESH ONE.
> 
> Wouldn't the syntax
> ALTER MATERIALIZED VIEW [ IF EXISTS ] name
> SET WITH NO DATA
> or
> SET WITHOUT DATA
> be better ?

We use "REFRESH ... WITH NO DATA" because there is already the syntax
to make materialized views non-scannable. We are just following in this.

https://www.postgresql.org/docs/12/sql-refreshmaterializedview.html

> 
> Continuing into this direction, did you ever think about an other feature
> like:  
> ALTER MATERIALIZED VIEW [ IF EXISTS ] name
> SET { NOINCREMENTAL }
> or even
> SET { NOINCREMENTAL | INCREMENTAL | INCREMENTAL CONCURRENTLY  }
> 
> that would permit to switch between those modes and would keep frozen data 
> available in the materialized view during heavy operations on source tables
> ?

Thank you for your suggestion! I agree that the feature to switch between
normal materialized view and incrementally maintainable view is useful.
We will add this to our ToDo list. Regarding its syntax, 
I would not like to add new keyword like NONINCREMENTAL, so how about 
the following

 ALTER MATERIALIZED VIEW ... SET {WITH | WITHOUT} INCREMENTAL REFRESH

although this is just a idea and we will need discussion on it.


Regards,
Yugo Nagata

-- 
Yugo NAGATA 

Re: 2020-02-13 Press Release Draft

[Jonathan S. Katz]

On 2/10/20 12:55 PM, Luís Roberto Weck wrote:
> Em 10/02/2020 13:46, Jonathan S. Katz escreveu:
>> Hi,
>>
>> Attached is a draft for the 2020-02-13 press release. I would appreciate
>> any review for accuracy, notable omissions, and the inevitable typos I
>> tend to have on drafts (even though I do run it through a spell
>> checker). There were over 75 fixes, so paring the list down was a bit
>> tricky, and I tried to focus on things that would have noticeable user
>> impact.
>>
>> As noted in other threads, this is the EOL release for 9.4. In a
>> departure from the past, I tried to give a bit of a "tribute" to 9.4 by
>> listing some of the major / impactful features that were introduced, the
>> thought process being that we should celebrate the history of PostgreSQL
>> and also look at how far we've come in 5 years. If we feel this does not
>> make sense, I'm happy to remove it.
>>
>> While I'll accept feedback up until time of release, please try to have
>> it in no later than 2020-02-13 0:00 UTC :)
>>
>> Thanks,
>>
>> Jonathan
> 
> s/Several fix for query planner errors/Several fixes for query planner
> errors/

Thanks! Fixed applied.

Here is the latest canonical copy. I also incorporated some of Alvaro's
suggestions, though when trying to add the query I found the explanation
becoming too long. Perhaps it might make an interesting blog post? ;)

Please let me know if there are any more suggestions/changes before the
release (which is rapidly approaching).

Thanks!

Jonathan

2020-02-13 Cumulative Update Release


The PostgreSQL Global Development Group has released an update to all supported
versions of our database system, including 12.2, 11.7, 10.12, 9.6.17, 9.5.21,
and 9.4.26. This release fixes over 75 bugs reported over the last three months.

Users should plan to upgrade at their earliest convenience.

PostgreSQL 9.4 Now EOL
--

This is the last release for PostgreSQL 9.4, which will no longer receive
security updates and bug fixes. [PostgreSQL 9.4 introduced new 
features](https://www.postgresql.org/about/news/1557/)
such as JSONB support, the `ALTER SYSTEM` command, the ability to stream logical
changes to an output plugin, [and 
more](https://www.postgresql.org/docs/9.4/release-9-4.html).

While we are very proud of this release, these features are also found in newer
versions of PostgreSQL, many of which have received improvements, and per our
[versioning policy](https://www.postgresql.org/support/versioning/), it is time
to retire PostgreSQL 9.4.

To receive continued support, We suggest that you make plans to upgrade to a
newer, supported version of PostgreSQL. Please see the PostgreSQL
[versioning policy](https://www.postgresql.org/support/versioning/) for more
information.

Bug Fixes and Improvements
--

This update also fixes over 75 bugs that were reported in the last several
months. Some of these issues affect only version 12, but may also affect all
supported versions.

Some of these fixes include:

* Fix for partitioned tables with foreign-key references where
`TRUNCATE ... CASCADE` would not remove all data. If you have previously used
`TRUNCATE ... CASCADE` on a partitioned table with foreign-key references,
please see the "Updating" section for verification and cleanup steps.
* Fix failure to add foreign key constraints to table with sub-partitions (aka a
multi-level partitioned table). If you have previously used this functionality,
you can fix it by either detaching and re-attaching the affected partition, or
by dropping and re-adding the foreign key constraint to the parent table. You
can find more information on how to perform these steps in the
[ALTER TABLE](https://www.postgresql.org/docs/current/sql-altertable.html)
documentation.
* Fix performance issue for partitioned tables introduced by the fix for
CVE-2017-7484 that now allows the planner to use statistics on a child table for
a column that the user is granted access to on the parent table when the query
contains a leaky operator.
* Several other fixes and changes for partitioned tables, including disallowing
partition key expressions that return pseudo-types, such as `RECORD`.
* Fix for logical replication subscribers for executing per-column `UPDATE`
triggers.
* Fix for several crashes and failures for logical replication subscribers and
publishers.
* Improve efficiency of logical replication with `REPLICA IDENTITY FULL`.
* Ensure that calling `pg_replication_slot_advance()` on a physical replication
slot will persist changes across restarts.
* Several fixes for the walsender processes.
* Improve performance of hash joins with very large inner relations.
* Fix placement of "Subplans Removed" field in EXPLAIN output by placing it with
its parent Append or MergeAppend plan.
* Several fixes for parallel query plans.
* Several fixes for query planner errors, including one that affected joins to
single-row subqueries.
* Several fixes for 

Re: error context for vacuum to include block number

[Masahiko Sawada]

On Sat, 8 Feb 2020 at 10:01, Justin Pryzby  wrote:
>
> On Tue, Feb 04, 2020 at 01:58:20PM +0900, Masahiko Sawada wrote:
> > Here is the comment for v16 patch:
> >
> > 2.
> > I think we can set the error context for heap scan again after
> > freespace map vacuum finishing, maybe after reporting the new phase.
> > Otherwise the user will get confused if an error occurs during
> > freespace map vacuum. And I think the comment is unclear, how about
> > "Set the error context fro heap scan again"?
>
> Good point
>
> > 3.
> > +   if (cbarg->blkno!=InvalidBlockNumber)
> > +   errcontext(_("while scanning block %u of relation 
> > \"%s.%s\""),
> > +   cbarg->blkno, cbarg->relnamespace, cbarg->relname);
> >
> > We can use BlockNumberIsValid macro instead.
>
> Thanks.  See attached, now squished together patches.
>
> I added functions to initialize the callbacks, so error handling is out of the
> way and minimally distract from the rest of vacuum.

Thank you for updating the patch! Here is the review comments:

1.
+static void vacuum_error_callback(void *arg);
+static void init_error_context_heap(ErrorContextCallback
*errcallback, vacuum_error_callback_arg *errcbarg, Relation onerel,
int phase);
+static void init_error_context_index(ErrorContextCallback
*errcallback, vacuum_error_callback_arg *errcbarg, Relation indrel,
int phase);

You need to add a newline to follow the limit line lengths so that the
code is readable in an 80-column window. Or please run pgindent.

2.
+/* Initialize error context for heap operations */
+static void
+init_error_context_heap(ErrorContextCallback *errcallback,
vacuum_error_callback_arg *errcbarg, Relation onerel, int phase)
+{
+   errcbarg->relnamespace = get_namespace_name(RelationGetNamespace(onerel));
+   errcbarg->relname = RelationGetRelationName(onerel);
+   errcbarg->indname = NULL; /* Not used for heap */
+   errcbarg->blkno = InvalidBlockNumber; /* Not known yet */
+   errcbarg->phase = phase;
+
+   errcallback->callback = vacuum_error_callback;
+   errcallback->arg = errcbarg;
+   errcallback->previous = error_context_stack;
+   error_context_stack = errcallback;
+}

I think that making initialization process of errcontext argument a
function is good. But maybe we can merge these two functions into one.
init_error_context_heap and init_error_context_index actually don't
only initialize the callback arguments but also push the vacuum
errcallback, in spite of the function name having 'init'. Also I think
it might be better to only initialize the callback arguments in this
function and to set errcallback by caller, rather than to wrap pushing
errcallback by a function. How about the following function
initializing the vacuum callback arguments?

static void
init_vacuum_error_callback_arg(vacuum_error_callback_arg *errcbarg,
Relation rel, int phase)
{
   errcbarg->relnamespace = get_namespace_name(RelationGetNamespace(onerel));
   errcbarg->blkno = InvalidBlockNumber;
   errcbarg->phase = phase;

   switch (phase) {
   case PROGRESS_VACUUM_PHASE_SCAN_HEAP:
   case PROGRESS_VACUUM_PHASE_VACUUM_HEAP:
   errcbarg->relname = RelationGetRelationName(rel);
   errcbarg->indname = NULL;
   break;

   case PROGRESS_VACUUM_PHASE_VACUUM_INDEX:
   case PROGRESS_VACUUM_PHASE_INDEX_CLEANUP:
   /* rel is an index relation in index vacuum case */
   errcbarg->relname = get_rel_name(indrel->rd_index->indexrelid);
   errcbarg->indname = RelationGetRelationName(rel);
   break;

   }
}

Regards,

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

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

[Dilip Kumar]

On Thu, Feb 13, 2020 at 9:46 AM Amit Kapila  wrote:
>
> On Tue, Feb 11, 2020 at 9:13 PM Tom Lane  wrote:
> >
> > I took a brief look through this patch.  I agree with the fundamental
> > idea that we shouldn't need to use the heavyweight lock manager for
> > relation extension, since deadlock is not a concern and no backend
> > should ever need to hold more than one such lock at once.  But it feels
> > to me like this particular solution is rather seriously overengineered.
> > I would like to suggest that we do something similar to Robert Haas'
> > excellent hack (daa7527af) for the !HAVE_SPINLOCK case in lmgr/spin.c,
> > that is,
> >
> > * Create some predetermined number N of LWLocks for relation extension.
> > * When we want to extend some relation R, choose one of those locks
> >   (say, R's relfilenode number mod N) and lock it.
> >
>
> I am imagining something on the lines of BufferIOLWLockArray (here it
> will be RelExtLWLockArray).  The size (N) could MaxBackends or some
> percentage of it (depending on testing) and indexing into an array
> could be as suggested (R's relfilenode number mod N).  We need to
> initialize this during shared memory initialization.  Then, to extend
> the relation with multiple blocks at-a-time (as we do in
> RelationAddExtraBlocks), we can either use the already proven
> technique of group clear xid mechanism (see ProcArrayGroupClearXid) or
> have an additional state in the RelExtLWLockArray which will keep the
> count of waiters (as done in latest patch of Sawada-san [1]).  We
> might want to experiment with both approaches and see which yields
> better results.

IMHO, in this case, there is no point in using the "group clear" type
of mechanism mainly for two reasons 1) It will unnecessarily make
PGPROC structure heavy.
2) For our case, we don't need any specific pieces of information from
other waiters, we just need the count.


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

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

[Amit Kapila]

On Tue, Feb 11, 2020 at 9:13 PM Tom Lane  wrote:
>
> I took a brief look through this patch.  I agree with the fundamental
> idea that we shouldn't need to use the heavyweight lock manager for
> relation extension, since deadlock is not a concern and no backend
> should ever need to hold more than one such lock at once.  But it feels
> to me like this particular solution is rather seriously overengineered.
> I would like to suggest that we do something similar to Robert Haas'
> excellent hack (daa7527af) for the !HAVE_SPINLOCK case in lmgr/spin.c,
> that is,
>
> * Create some predetermined number N of LWLocks for relation extension.
> * When we want to extend some relation R, choose one of those locks
>   (say, R's relfilenode number mod N) and lock it.
>

I am imagining something on the lines of BufferIOLWLockArray (here it
will be RelExtLWLockArray).  The size (N) could MaxBackends or some
percentage of it (depending on testing) and indexing into an array
could be as suggested (R's relfilenode number mod N).  We need to
initialize this during shared memory initialization.  Then, to extend
the relation with multiple blocks at-a-time (as we do in
RelationAddExtraBlocks), we can either use the already proven
technique of group clear xid mechanism (see ProcArrayGroupClearXid) or
have an additional state in the RelExtLWLockArray which will keep the
count of waiters (as done in latest patch of Sawada-san [1]).  We
might want to experiment with both approaches and see which yields
better results.

[1] - 
https://www.postgresql.org/message-id/CAD21AoADkWhkLEB_%3DkjLZeZ_ML9_hSQqNBWz%2Bd821QHf%3DO9LJQ%40mail.gmail.com
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Re: Internal key management system

[Masahiko Sawada]

On Tue, 11 Feb 2020 at 10:57, Andres Freund  wrote:
>
> Hi,
>
> On 2020-02-08 12:07:57 -0500, Tom Lane wrote:
> > For the same reason, I don't think that an "internal key management"
> > feature in the core code is ever going to be acceptable.  It has to
> > be an extension.  (But, as long as it's an extension, whether it's
> > bringing its own crypto or relying on some other extension for that
> > doesn't matter from the legal standpoint.)
>
> I'm not convinced by that. We have optional in-core functionality that
> requires external libraries, and we add more cases, if necessary. Given
> that the goal of this work is to be useful for on-disk encryption, I
> don't see moving it into an extension being viable?

As far as I researched it is significantly hard to implement
transparent data encryption without introducing into core. Adding a
hook to the point where flushing data to the disk for encryption,
compression and tracking dirty blocks has ever been proposed but it
has been rejected every time.

>
> I am somewhat doubtful that the, imo significant, complexity of the
> feature is worth it, but that's imo a different discussion.
>
>
> > > Sure, I know the code is currently calling ooenssl functions. I was
> > > responding to Masahiko-san's message that we might eventually merge this
> > > openssl code into our tree.
> >
> > No.  This absolutely, positively, will not happen.  There will never be
> > crypto functions in our core tree, because then there'd be no recourse for
> > people who want to use Postgres in countries with restrictions on crypto
> > software.  It's hard enough for them that we have such code in contrib
> > --- but at least they can remove pgcrypto and be legal.  If it's in
> > src/common then they're stuck
>
> Isn't that basically a problem of the past by now?  Partially due to
> changed laws (e.g. France, which used to be a problematic case), but
> also because it's basically futile to have import restrictions on
> cryptography by now. Just about every larger project contains
> significant amounts of cryptographic code and it's entirely impractical
> to operate anything interfacing with network without some form of
> transport encryption.  And just about all open source distribution
> mechanism have stopped separating out crypto code a long time ago.
>
> I however do agree that we should strive to not introduce cryptographic
> code into the pg source tree

It doesn't include the case where we introduce the code using openssl
cryptographic function library to the core. Is that right?

Regards,

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

Re: Bug in pg_restore with EventTrigger in parallel mode

[Michael Paquier]

On Wed, Feb 12, 2020 at 01:59:05PM -0300, Fabrízio de Royes Mello wrote:
> In parallel mode it's firing the EventTrigger and it can't be happen.
> Poking around it I did some test with attached just to leave EventTriggers
> in pending_list to process it in restore_toc_entries_postfork and
> everything is ok, but my solution is very ugly, so maybe we need to invent
> a new RestorePass to take care of it like RESTORE_PASS_ACL and
> RESTORE_PASS_REFRESH. I can provide a more polished patch if it'll be a
> good way to do that.

Could you add that as a bug fix to the next CF [1]?

[1]: https://commitfest.postgresql.org/27/
--
Michael


signature.asc
Description: PGP signature

Re: Just for fun: Postgres 20?

[Michael Paquier]

On Wed, Feb 12, 2020 at 09:46:48AM -0500, Tom Lane wrote:
> Yeah; I don't think it's *that* unlikely for it to happen again.  But
> my own principal concern about this mirrors what somebody else already
> pointed out: the one-major-release-per-year schedule is not engraved on
> any stone tablets.  So I don't want to go to a release numbering system
> that depends on us doing it that way for the rest of time.

Yeah, it is good to keep some flexibility here, so my take is that
there is little advantage in changing again the version numbering.
Note that any change like that induces an extra cost for anybody
maintaining builds of Postgres or any upgrade logic where the decision
depends on the version number of the origin build and the target
build.
--
Michael


signature.asc
Description: PGP signature

Re: Unicode normalization SQL functions

[Michael Paquier]

On Thu, Feb 13, 2020 at 01:23:41AM +0100, Andreas Karlsson wrote:
> On 1/28/20 9:21 PM, Peter Eisentraut wrote:
>> You're right, this didn't make any sense.  Here is a new patch set with
>> that fixed.
> 
> Thanks for this patch. This is a feature which has been on my personal todo
> list for a while and something which I have wished to have a couple of
> times.

(The size of the patch set may justify compressing it)
--
Michael


signature.asc
Description: PGP signature

Re: Exposure related to GUC value of ssl_passphrase_command

[Michael Paquier]

On Thu, Feb 13, 2020 at 11:28:05AM +0900, Kyotaro Horiguchi wrote:
> I think it is reasonable.

Indeed, that makes sense to me as well.  I am adding Peter Eisentraut
in CC as the author/committer of 8a3d942 to comment on that.

> By the way, I'm not sure the criteria of setting a GUC variable as
> GUC_SUPERUSER_ONLY, but for example, ssl_max/min_protocol_version,
> dynamic_library_path, log_directory, krb_server_keyfile,
> data_directory and config_file are GUC_SUPERUSER_ONLY. So, it seems to
> me very strange that ssl_*_file are not. Don't we need to mark them
> maybe and some of the other ssl_* as the same?

This should be a separate discussion IMO.  Perhaps there is a point in
softening or hardening some of them.
--
Michael


signature.asc
Description: PGP signature

Re: Wait event that should be reported while waiting for WAL archiving to finish

[Michael Paquier]

On Thu, Feb 13, 2020 at 02:29:20AM +0900, Fujii Masao wrote:
> When I saw pg_stat_activity.wait_event while pg_basebackup -X none
> is waiting for WAL archiving to finish, it was either NULL or
> CheckpointDone. I think this is confusing. What about introducing
> new wait_event like WAIT_EVENT_BACKUP_WAIT_WAL_ARCHIVE
> (BackupWaitWalArchive) and reporting it during that period?

Sounds like a good idea to me.  You need to be careful that this does
not overwrite more low-level wait event registration though, so that
could be more tricky than it looks at first sight.
--
Michael


signature.asc
Description: PGP signature

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

[Amit Kapila]

On Wed, Feb 12, 2020 at 10:23 PM Tom Lane  wrote:
>
> Amit Kapila  writes:
> > On Wed, Feb 12, 2020 at 7:36 AM Masahiko Sawada
> >  wrote:
> >> On Wed, 12 Feb 2020 at 00:43, Tom Lane  wrote:
> >>> I would like to suggest that we do something similar to Robert Haas'
> >>> excellent hack (daa7527af) for the !HAVE_SPINLOCK case in lmgr/spin.c,
>
> >> My original proposal used LWLocks and hash tables for relation
> >> extension but there was a discussion that using LWLocks is not good
> >> because it's not interruptible[1].
>
> > Hmm, but we use LWLocks for (a) WALWrite/Flush (see the usage of
> > WALWriteLock), (b) writing the shared buffer contents (see
> > io_in_progress lock and its usage in FlushBuffer) and might be for few
> > other similar stuff.  Many times those take more time than extending a
> > block in relation especially when we combine the WAL write for
> > multiple commits.  So, if this is a problem for relation extension
> > lock, then the same thing holds true there also.
>
> Yeah.  I would say a couple more things:
>
> * I see no reason to think that a relation extension lock would ever
> be held long enough for noninterruptibility to be a real issue.  Our
> expectations for query cancel response time are in the tens to
> hundreds of msec anyway.
>
> * There are other places where an LWLock can be held for a *long* time,
> notably the CheckpointLock.  If we do think this is an issue, we could
> devise a way to not insist on noninterruptibility.  The easiest fix
> is just to do a matching RESUME_INTERRUPTS after getting the lock and
> HOLD_INTERRUPTS again before releasing it; though maybe it'd be worth
> offering some slightly cleaner way.
>

Yeah, this sounds like the better answer for noninterruptibility
aspect of this design.  One idea that occurred to me was to pass a
parameter to LWLOCK acquire/release APIs to indicate whether to
hold/resume interrupts, but I don't know if that is any better than
doing it at the required place.  I am not sure if all places are
careful whether they really want to hold interrupts, so if we provide
a new parameter at least new users of API will think about it.

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

Re: Exposure related to GUC value of ssl_passphrase_command

[Kyotaro Horiguchi]

At Thu, 13 Feb 2020 02:37:29 +0900, Fujii Masao  wrote 
in 
> On Fri, Nov 8, 2019 at 4:24 PM Amit Langote  wrote:
> >
> > Hello.
> >
> > On Tue, Nov 5, 2019 at 5:15 PM Moon, Insung  
> > wrote:
> > > Deal Hackers.
> > >
> > > The value of ssl_passphrase_command is set so that an external command
> > > is called when the passphrase for decrypting an SSL file such as a
> > > private key is obtained.
> > > Therefore, easily set to work with echo "passphrase" or call to
> > > another get of passphrase application.
> > >
> > > I think that this GUC value doesn't contain very sensitive data,
> > > but just in case, it's dangerous to be visible to all users.
> > > I think do not possible these cases, but if a used echo external
> > > commands or another external command,  know what application used to
> > > get the password, maybe we can't be convinced that there's the safety
> > > of using abuse by backtracking on applications.
> > > So I think to the need only superusers or users with the default role
> > > of pg_read_all_settings should see these values.
> > >
> > > Patch is very simple.
> > > How do you think about my thoughts like this?
> >
> > I'm hardly an expert on this topic, but reading this blog post about
> > ssl_passphrase_command:
> >
> > https://www.2ndquadrant.com/en/blog/postgresql-passphrase-protected-ssl-keys-systemd/
> >
> > which mentions that some users might go with the very naive
> > configuration such as:
> >
> > ssl_passphrase_command = 'echo "secret"'
> >
> > maybe it makes sense to protect its value from everyone but superusers.
> >
> > So +1.
> 
> Seems this proposal is reasonable.

I think it is reasonable.

By the way, I'm not sure the criteria of setting a GUC variable as
GUC_SUPERUSER_ONLY, but for example, ssl_max/min_protocol_version,
dynamic_library_path, log_directory, krb_server_keyfile,
data_directory and config_file are GUC_SUPERUSER_ONLY. So, it seems to
me very strange that ssl_*_file are not. Don't we need to mark them
maybe and some of the other ssl_* as the same?

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: Getting rid of some more lseek() calls

[Thomas Munro]

On Thu, Feb 13, 2020 at 5:30 AM Alvaro Herrera  wrote:
> On 2020-Feb-12, Thomas Munro wrote:
> > Hmm.  Well, on Unix we have to choose between "tell me the size but
> > also change the position that I either don't care about or have to
> > undo", and "tell me the size but also tell me all this other stuff I
> > don't care about".  Since Windows apparently has GetFileSizeEx(), why
> > not use that when that's exactly what you want?  It apparently
> > understands large files.
>
> I was already thinking that it might be better to make the new function
> just "tell me the file size" without leaking the details of *how* we do
> it, before reading about this Windows call.  That reinforces it, IMO.

Ok, how about this?


0001-Introduce-pg_file_size-to-get-the-file-size-for-an-f.patch
Description: Binary data


0002-Remove-lseek-calls-from-slru.c.patch
Description: Binary data


0003-Remove-lseek-calls-from-walsender.c.patch
Description: Binary data

Re: Identifying user-created objects

[Kyotaro Horiguchi]

At Mon, 10 Feb 2020 14:32:44 +0900, Amit Langote  
wrote in 
> On Mon, Feb 10, 2020 at 2:23 PM Masahiko Sawada
>  wrote:
> > On Mon, 10 Feb 2020 at 14:09, Michael Paquier  wrote:
> > >
> > > On Mon, Feb 10, 2020 at 01:16:30PM +0900, Amit Langote wrote:
> > > > On Mon, Feb 10, 2020 at 1:06 PM Masahiko Sawada
> > > >  wrote:
> > > >> How about having it as a macro like:
> > > >>
> > > >> #define ObjectIdIsUserObject(oid) ((Oid)(oid) >= FirstNormalObjectId)
> > > >
> > > > I'm fine with a macro.
> > >
> > > I am not sure that it is worth having one extra abstraction layer for
> > > that.
> >
> > Hmm I'm not going to insist on that but I thought that it could
> > somewhat improve readability at places where they already compares an
> > oid to FirstNormalObjectId as Amit mentioned:
> >
> > src/backend/catalog/pg_publication.c:   relid >= FirstNormalObjectId;
> > src/backend/utils/adt/json.c:   if (typoid >= 
> > FirstNormalObjectId)
> > src/backend/utils/adt/jsonb.c:  if (typoid >= 
> > FirstNormalObjectId)
> 
> Agree that ObjectIsUserObject(oid) is easier to read than oid >=
> FirstNormalObject.  I would have not bothered, for example, if it was
> something like oid >= FirstUserObjectId to begin with.

Aside from the naming, I'm not sure it's sensible to use
FirstNormalObjectId since I don't see a clear definition or required
characteristics for "user created objects" is.  If we did CREATE
TABLE, FUNCTION or maybe any objects during single-user mode before
the first object is created during normal multiuser operation, the
"user-created(or not?)" object has an OID less than
FirstNormalObjectId. If such objects are the "user created object", we
need FirstUserObjectId defferent from FirstNormalObjectId.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: [PATCH] libpq improvements and fixes

[Tom Lane]

Ranier Vilela  writes:
> Coverity detected a dead code in the src / interfaces / libpq / fe-auth.c
> file, to correct it, a simplification was made and the oom_error goto was
> removed, since it is clearly redundant and its presence can be confusing.

I'm kind of disinclined to let Coverity dictate our coding style here.
We've dismissed many hundreds of its reports as false positives, and
this seems like one that could get (probably already has gotten) the
same treatment.  I also don't feel like duplicating error messages
as you propose is an improvement.

If we did want to adjust the code in pg_SASL_init, my thought would
be to reduce not increase the code duplication, by making the error
exits look like

...
return STATUS_OK;

oom_error:
printfPQExpBuffer(>errorMessage,
  libpq_gettext("out of memory\n"));
/* FALL THRU */

error:
termPQExpBuffer(_buf);
if (initialresponse)
free(initialresponse);
return STATUS_ERROR;
}

It's only marginally worth the trouble though.

> First, a correction was made to the return types of some functions that
> clearly return bool, but are defined as int.

This is ancient history that doesn't seem worth revisiting.  There is
certainly exactly zero chance of us changing libpq's external API
as you propose, because of the ensuing ABI breakage.  Maybe we could
change the static functions, but I'm not very excited about it.

I can't get excited about the other code rearrangements you're proposing
here either.  They seem to make the code more intellectually complex for
little benefit.

regards, tom lane

Re: Unicode normalization SQL functions

[Andreas Karlsson]

On 1/28/20 9:21 PM, Peter Eisentraut wrote:
You're right, this didn't make any sense.  Here is a new patch set with 
that fixed.


Thanks for this patch. This is a feature which has been on my personal 
todo list for a while and something which I have wished to have a couple 
of times.


I took a quick look at the patch and here is some feedback:

A possible concern is increased binary size from the new tables for the 
quickcheck but personally I think they are worth it.


A potential optimization would be to merge utf8_to_unicode() and 
pg_utf_mblen() into one function in unicode_normalize_func() since 
utf8_to_unicode() already knows length of the character. Probably not 
worth it though.


It feels a bit wasteful to measure output_size in 
unicode_is_normalized() since unicode_normalize() actually already knows 
the length of the buffer, it just does not return it.


A potential optimization for the normalized case would be to abort the 
quick check on the first maybe and normalize from that point on only. If 
I can find the time I might try this out and benchmark it.


Nitpick: "split/\s*;\s*/, $line" in generate-unicode_normprops_table.pl 
should be "split /\s*;\s*/, $line".


What about using else if in the code below for clarity?

+   if (check == UNICODE_NORM_QC_NO)
+   return UNICODE_NORM_QC_NO;
+   if (check == UNICODE_NORM_QC_MAYBE)
+   result = UNICODE_NORM_QC_MAYBE;

Remove extra space in the line below.

+   else if (quickcheck == UNICODE_NORM_QC_NO )

Andreas

[PATCH] libpq improvements and fixes

[Ranier Vilela]

Hi,
Coverity detected a dead code in the src / interfaces / libpq / fe-auth.c
file, to correct it, a simplification was made and the oom_error goto was
removed, since it is clearly redundant and its presence can be confusing.

The second part of the patch refers to the file src / interfaces / libpq /
fe-exec.c.
First, a correction was made to the return types of some functions that
clearly return bool, but are defined as int.

According to some functions, they do a basic check and if they fail, they
return immediately, so it does not make sense to start communication and
then return.
It makes more sense to do the basic checks, only to start communicating
with the server afterwards.

These changes are passing the regression tests and are in use in libpq.dll,
used in production by my customers.

regards,
Ranier Vilela


libpq.patch
Description: Binary data

Re: Just for fun: Postgres 20?

[Michael Banck]

Hi,

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

It was my undestanding that this prompted us to form the release team,
which has since done a great job of making sure that this does not
happen again.

Of course, this does not mean it won't ever happen again. Even then,
shipping PostgreSQL 23 at the beginning of 2024 wouldn't be a total
disaster in my opinion.

The fact that the community might want to re-think the major release
cycle at some point and not be tied to yearly release numbers is the
most convincing argument against it.

That, and the PR-style "sell-out" it might be regarded as.


Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer

Unser Umgang mit personenbezogenen Daten unterliegt
folgenden Bestimmungen: https://www.credativ.de/datenschutz

Re: Just for fun: Postgres 20?

[Ray O'Donnell]

On 12/02/2020 21:10, David Fetter wrote:
> On Wed, Feb 12, 2020 at 05:25:15PM +0100, Juan José Santamaría Flecha wrote:
>> On Wed, Feb 12, 2020 at 3:47 PM Tom Lane  wrote:
>>
>>>
>>> Yeah; I don't think it's *that* unlikely for it to happen again.  But
>>> my own principal concern about this mirrors what somebody else already
>>> pointed out: the one-major-release-per-year schedule is not engraved on
>>> any stone tablets.  So I don't want to go to a release numbering system
>>> that depends on us doing it that way for the rest of time.
>>>
>>>
>> We could you use  as version identifier, so people will not expect
>> correlative numbering. SQL Server is being released every couple of years
>> and they are using this naming shema. The problem would be releasing twice
>> the same year, but how likely would that be?
> 
> We've released more than one major version in a year before, so we
> have a track record of that actually happening.

Besides what everyone else has said, it's not that long since the
numbering scheme was changed for major versions. Changing it again so
soon would, IMHO, look confused at best.

Ray.

-- 
Raymond O'Donnell // Galway // Ireland
r...@rodonnell.ie

Re: Just for fun: Postgres 20?

[David Fetter]

On Wed, Feb 12, 2020 at 05:25:15PM +0100, Juan José Santamaría Flecha wrote:
> On Wed, Feb 12, 2020 at 3:47 PM Tom Lane  wrote:
> 
> >
> > Yeah; I don't think it's *that* unlikely for it to happen again.  But
> > my own principal concern about this mirrors what somebody else already
> > pointed out: the one-major-release-per-year schedule is not engraved on
> > any stone tablets.  So I don't want to go to a release numbering system
> > that depends on us doing it that way for the rest of time.
> >
> >
> We could you use  as version identifier, so people will not expect
> correlative numbering. SQL Server is being released every couple of years
> and they are using this naming shema. The problem would be releasing twice
> the same year, but how likely would that be?

We've released more than one major version in a year before, so we
have a track record of that actually happening.

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

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

Re: Collation versioning

[Thomas Munro]

On Thu, Feb 13, 2020 at 9:16 AM Julien Rouhaud  wrote:
> On Wed, Feb 12, 2020 at 08:55:06PM +0100, Laurenz Albe wrote:
> > I didn't study the patch in detail, but do I get it right that there will 
> > be no
> > warnings about version incompatibilities with libc collations?
>
> No, libc is also be supported (including the default collation), as long as we
> have a way to get the version.  Unfortunately, that means only linux/glibc.  I
> think that there was some previous discussion to work around that limitation
> for other systems, using some kind of hash of the underlying collation files,
> as Peter mentioned recently, but that's not part of this patchset.

Yeah, this is about the cataloguing infrastructure part, to get the
model and mechanisms right.  To actually get version information from
the underlying collation provider, there will need to be a series of
per-OS projects.  For glibc right now, it's done, but we just use the
whole glibc version as a proxy (sadly we know this can give false
positives and false negatives, but is expected to work a lot better
than nothing).  I hope we can get a proper CLDR version out of that
library one day.  For FreeBSD libc, I have patches, I just need more
round tuits.  For Windows, there is
https://commitfest.postgresql.org/27/2351/ which I'm planning to
commit soonish, after some more thought about the double-version
thing.  Then there is the "run a user-supplied script that gives me a
version" concept, which might work and perhaps allow package
maintainers to supply a script that works on each system.  Again,
that'd be a separate project.  I guess there will probably always be
some OSes that we can't get the data from so we'll probably always
have to support "don't know" mode.

Re: Collation versioning

[Julien Rouhaud]

On Wed, Feb 12, 2020 at 08:55:06PM +0100, Laurenz Albe wrote:
> On Wed, 2020-02-12 at 20:13 +0100, Julien Rouhaud wrote:
> > On Wed, Feb 05, 2020 at 05:17:25PM +0100, Julien Rouhaud wrote:
> > > Note that I didn't change any syntax (or switched to native functions
> > > for the binary pg_dump) as it's still not clear to me what exactly
> > > should be implemented.
> >
> > Hearing no complaints on the suggestions, I'm attaching v8 to address that:
> >
> > - pg_dump is now using a binary_upgrade_set_index_coll_version() function
> >   rather than plain DDL
> > - the additional DDL is now of the form:
> >   ALTER INDEX name ALTER COLLATION name REFRESH VERSION
> >
> > I also added an alternate file for the collate.icu.utf8, so the build farm 
> > bot
> > should turn green for the linux part.
>
> diff --git a/doc/src/sgml/ref/alter_index.sgml 
> b/doc/src/sgml/ref/alter_index.sgml
> index 6d34dbb74e..8661b031e9 100644
> --- a/doc/src/sgml/ref/alter_index.sgml
> +++ b/doc/src/sgml/ref/alter_index.sgml
> @@ -109,6 +110,18 @@ ALTER INDEX ALL IN TABLESPACE  class="parameter">name
>  
> 
>
> +   
> +ALTER COLLATION
> +
> + 
> +  This form update the index existing dependency on a specific collation,
> +  to specificy the the currently installed collation version is 
> compatible
> +  with the version used the last time the index was built.  Be aware that
> +  an incorrect use of this form can hide a corruption on the index.
> + 
> +
> +   
> +
> 
>  SET (  class="parameter">storage_parameter =  class="parameter">value [, ... ] )
>  
>
> This description could do with some love.  Perhaps:
>
> This command declares that the index is compatible with the currently
> installed version of the collations that determine its order.  It is used
> to silence warnings caused by collation
> version incompatibilities and
> should be called after rebuilding the index or otherwise verifying its
> consistency.  Be aware that incorrect use of this command can hide
> index corruption.

Thanks a lot, that's indeed way better!  I'll add it in the round of patch.

> I didn't study the patch in detail, but do I get it right that there will be 
> no
> warnings about version incompatibilities with libc collations?

No, libc is also be supported (including the default collation), as long as we
have a way to get the version.  Unfortunately, that means only linux/glibc.  I
think that there was some previous discussion to work around that limitation
for other systems, using some kind of hash of the underlying collation files,
as Peter mentioned recently, but that's not part of this patchset.

Re: Just for fun: Postgres 20?

[Isaac Morland]

On Wed, 12 Feb 2020 at 14:58, Laurenz Albe  wrote:

> On Wed, 2020-02-12 at 12:32 -0500, Christopher Browne wrote:
> > All said, I think there's some merit to avoiding a PostgreSQL 13
> release, because
> > there's enough superstition out there about the infamous "number 13."
>
> It would make me sad if the project kotowed to superstition like Oracle
> did.
>

Agreed. That being said, everybody knows you can't avoid the curse of 13 by
re-numbering it - you simply have to avoid the version/floor/day/whatever
after 12.

Re: Just for fun: Postgres 20?

[Laurenz Albe]

On Wed, 2020-02-12 at 12:32 -0500, Christopher Browne wrote:
> All said, I think there's some merit to avoiding a PostgreSQL 13 release, 
> because
> there's enough superstition out there about the infamous "number 13."

It would make me sad if the project kotowed to superstition like Oracle did.

Yours,
Laurenz Albe

Re: Collation versioning

[Laurenz Albe]

On Wed, 2020-02-12 at 20:13 +0100, Julien Rouhaud wrote:
> On Wed, Feb 05, 2020 at 05:17:25PM +0100, Julien Rouhaud wrote:
> > Note that I didn't change any syntax (or switched to native functions
> > for the binary pg_dump) as it's still not clear to me what exactly
> > should be implemented.
> 
> Hearing no complaints on the suggestions, I'm attaching v8 to address that:
> 
> - pg_dump is now using a binary_upgrade_set_index_coll_version() function
>   rather than plain DDL
> - the additional DDL is now of the form:
>   ALTER INDEX name ALTER COLLATION name REFRESH VERSION
> 
> I also added an alternate file for the collate.icu.utf8, so the build farm bot
> should turn green for the linux part.

diff --git a/doc/src/sgml/ref/alter_index.sgml 
b/doc/src/sgml/ref/alter_index.sgml
index 6d34dbb74e..8661b031e9 100644
--- a/doc/src/sgml/ref/alter_index.sgml
+++ b/doc/src/sgml/ref/alter_index.sgml
@@ -109,6 +110,18 @@ ALTER INDEX ALL IN TABLESPACE name
 

 
+   
+ALTER COLLATION
+
+ 
+  This form update the index existing dependency on a specific collation,
+  to specificy the the currently installed collation version is compatible
+  with the version used the last time the index was built.  Be aware that
+  an incorrect use of this form can hide a corruption on the index.
+ 
+
+   
+

 SET ( storage_parameter = value [, ... ] )
 

This description could do with some love.  Perhaps:

This command declares that the index is compatible with the currently
installed version of the collations that determine its order.  It is used
to silence warnings caused by collation
version incompatibilities and
should be called after rebuilding the index or otherwise verifying its
consistency.  Be aware that incorrect use of this command can hide
index corruption.

I didn't study the patch in detail, but do I get it right that there will be no
warnings about version incompatibilities with libc collations?

Yours,
Laurenz Albe

Re: In PG12, query with float calculations is slower than PG11

[Emre Hasegeli]

> > > For most places it'd probably end up being easier to read and to
> > > optimize if we just wrote them as
> > > if (unlikely(isinf(result)) && !isinf(arg))
> > > float_overflow_error();
> > > and when needed added a
> > > else if (unlikely(result == 0) && arg1 != 0.0)
> > > float_underflow_error();
> >
> > +1
>
> Cool. Emre, any chance you could write a patch along those lines?

Yes, I am happy to do.  It makes more sense to me too.

Re: In PG12, query with float calculations is slower than PG11

[Tom Lane]

Andres Freund  writes:
> I'm inclined that we should backpatch that, and just leave the inline
> function (without in core callers) in place in 12?

Yeah, we can't remove the inline function in 12.  But we don't have
to use it.

regards, tom lane

Re: In PG12, query with float calculations is slower than PG11

[Andres Freund]

Hi,

On 2020-02-12 14:18:30 -0500, Tom Lane wrote:
> Andres Freund  writes:
> > I do wonder if we're just punching ourselves in the face with the
> > signature of these checks. Part of the problem here really comes from
> > using the same function to handle a number of different checks.
> 
> Yeah, I've thought that too.  It's *far* from clear that this thing
> is a win at all, other than your point about the number of copies of
> the ereport call.  It's bulky, it's hard to optimize, and I have
> never thought it was more readable than the direct tests it replaced.
> 
> > For most places it'd probably end up being easier to read and to
> > optimize if we just wrote them as
> > if (unlikely(isinf(result)) && !isinf(arg))
> > float_overflow_error();
> > and when needed added a
> > else if (unlikely(result == 0) && arg1 != 0.0)
> > float_underflow_error();
> 
> +1

Cool. Emre, any chance you could write a patch along those lines?

I'm inclined that we should backpatch that, and just leave the inline
function (without in core callers) in place in 12?

Greetings,

Andres Freund

Re: In PG12, query with float calculations is slower than PG11

[Tom Lane]

Andres Freund  writes:
> I do wonder if we're just punching ourselves in the face with the
> signature of these checks. Part of the problem here really comes from
> using the same function to handle a number of different checks.

Yeah, I've thought that too.  It's *far* from clear that this thing
is a win at all, other than your point about the number of copies of
the ereport call.  It's bulky, it's hard to optimize, and I have
never thought it was more readable than the direct tests it replaced.

> For most places it'd probably end up being easier to read and to
> optimize if we just wrote them as
> if (unlikely(isinf(result)) && !isinf(arg))
> float_overflow_error();
> and when needed added a
> else if (unlikely(result == 0) && arg1 != 0.0)
> float_underflow_error();

+1

regards, tom lane

Re: In PG12, query with float calculations is slower than PG11

[Andres Freund]

Hi,

On 2020-02-12 13:15:22 -0500, Tom Lane wrote:
> Andres Freund  writes:
> > I'd just rename the macro to the name of the inline function. No need to
> > have a verbose change in all callsites just to update the name imo.
>
> +1, that's what I had in mind too.  That does suggest though that we
> ought to make sure the macro has single-eval behavior, so that you
> don't need to know it's a macro.

We'd have to store 'val' in a local variable for that I think. Not the
prettiest, but also not a problem.


I do wonder if we're just punching ourselves in the face with the
signature of these checks. Part of the problem here really comes from
using the same function to handle a number of different checks.

I mean something like dtof's
check_float4_val((float4) num, isinf(num), num == 0);
where the num == 0 is solely to satisfy the check function is a bit
stupid.

And the reason we have these isinf(arg1) || isinf(arg2) parameters is
also largely because we force the same function to be used in cases
where we have two inputs, rather than just one.

For most places it'd probably end up being easier to read and to
optimize if we just wrote them as

if (unlikely(isinf(result)) && !isinf(arg))
float_overflow_error();

and when needed added a

else if (unlikely(result == 0) && arg1 != 0.0)
float_underflow_error();

the verbose piece really is the error, not the error check. Sure, there
are more complicated cases like

if (unlikely(isinf(result)) && (!isinf(arg1) || !isinf(arg2)))

but that's still not very complicated.

Greetings,

Andres Freund

assert pg_class.relnatts is consistent

[Justin Pryzby]

Forking this thread for two tangential patches which I think are more
worthwhile than the original topic's patch.
https://www.postgresql.org/message-id/20200207143935.GP403%40telsasoft.com

Is there a better place to implement assertion from 0002 ?

On Fri, Feb 07, 2020 at 08:39:35AM -0600, Justin Pryzby wrote:
> From 7eea0a17e495fe13379ffd589b551f2f145f5672 Mon Sep 17 00:00:00 2001
> From: Justin Pryzby 
> Date: Thu, 6 Feb 2020 21:48:13 -0600
> Subject: [PATCH v1 1/3] Update comment obsolete since b9b8831a
> 
> ---
>  src/backend/commands/cluster.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
> index e9d7a7f..3adcbeb 100644
> --- a/src/backend/commands/cluster.c
> +++ b/src/backend/commands/cluster.c
> @@ -1539,9 +1539,9 @@ get_tables_to_cluster(MemoryContext cluster_context)
>  
>   /*
>* Get all indexes that have indisclustered set and are owned by
> -  * appropriate user. System relations or nailed-in relations cannot ever
> -  * have indisclustered set, because CLUSTER will refuse to set it when
> -  * called with one of them as argument.
> +  * appropriate user. Shared relations cannot ever have indisclustered
> +  * set, because CLUSTER will refuse to set it when called with one as
> +  * an argument.
>*/
>   indRelation = table_open(IndexRelationId, AccessShareLock);
>   ScanKeyInit(,
> -- 
> 2.7.4
> 

> From 4777be522a7aa8b8c77b13f765cbd02043438f2a Mon Sep 17 00:00:00 2001
> From: Justin Pryzby 
> Date: Fri, 7 Feb 2020 08:12:50 -0600
> Subject: [PATCH v1 2/3] Give developer a helpful kick in the pants if they
>  change natts in one place but not another
> 
> ---
>  src/backend/bootstrap/bootstrap.c | 23 +++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/src/backend/bootstrap/bootstrap.c 
> b/src/backend/bootstrap/bootstrap.c
> index bfc629c..d5e1888 100644
> --- a/src/backend/bootstrap/bootstrap.c
> +++ b/src/backend/bootstrap/bootstrap.c
> @@ -25,7 +25,9 @@
>  #include "access/xlog_internal.h"
>  #include "bootstrap/bootstrap.h"
>  #include "catalog/index.h"
> +#include "catalog/pg_class.h"
>  #include "catalog/pg_collation.h"
> +#include "catalog/pg_proc.h"
>  #include "catalog/pg_type.h"
>  #include "common/link-canary.h"
>  #include "libpq/pqsignal.h"
> @@ -49,6 +51,7 @@
>  #include "utils/ps_status.h"
>  #include "utils/rel.h"
>  #include "utils/relmapper.h"
> +#include "utils/syscache.h"
>  
>  uint32   bootstrap_data_checksum_version = 0;/* No checksum 
> */
>  
> @@ -602,6 +605,26 @@ boot_openrel(char *relname)
>   TableScanDesc scan;
>   HeapTuple   tup;
>  
> + /* Check that pg_class data is consistent now, rather than failing 
> obscurely later */
> + struct { Oid oid; int natts; }
> + checknatts[] = {
> + {RelationRelationId, Natts_pg_class,},
> + {TypeRelationId, Natts_pg_type,},
> + {AttributeRelationId, Natts_pg_attribute,},
> + {ProcedureRelationId, Natts_pg_proc,},
> + };
> +
> + for (int i=0; i + Form_pg_class   classForm;
> + HeapTuple   tuple;
> + tuple = SearchSysCache1(RELOID, 
> ObjectIdGetDatum(checknatts[i].oid));
> + if (!HeapTupleIsValid(tuple))
> + elog(ERROR, "cache lookup failed for relation %u", 
> checknatts[i].oid);
> + classForm = (Form_pg_class) GETSTRUCT(tuple);
> + Assert(checknatts[i].natts == classForm->relnatts);
> + ReleaseSysCache(tuple);
> + }
> +
>   if (strlen(relname) >= NAMEDATALEN)
>   relname[NAMEDATALEN - 1] = '\0';
>  
> -- 
> 2.7.4
> 
>From 4777be522a7aa8b8c77b13f765cbd02043438f2a Mon Sep 17 00:00:00 2001
From: Justin Pryzby 
Date: Fri, 7 Feb 2020 08:12:50 -0600
Subject: [PATCH v1 2/3] Give developer a helpful kick in the pants if they
 change natts in one place but not another

---
 src/backend/bootstrap/bootstrap.c | 23 +++
 1 file changed, 23 insertions(+)

diff --git a/src/backend/bootstrap/bootstrap.c b/src/backend/bootstrap/bootstrap.c
index bfc629c..d5e1888 100644
--- a/src/backend/bootstrap/bootstrap.c
+++ b/src/backend/bootstrap/bootstrap.c
@@ -25,7 +25,9 @@
 #include "access/xlog_internal.h"
 #include "bootstrap/bootstrap.h"
 #include "catalog/index.h"
+#include "catalog/pg_class.h"
 #include "catalog/pg_collation.h"
+#include "catalog/pg_proc.h"
 #include "catalog/pg_type.h"
 #include "common/link-canary.h"
 #include "libpq/pqsignal.h"
@@ -49,6 +51,7 @@
 #include "utils/ps_status.h"
 #include "utils/rel.h"
 #include "utils/relmapper.h"
+#include "utils/syscache.h"
 
 uint32		bootstrap_data_checksum_version = 0;	/* No checksum */
 
@@ -602,6 +605,26 @@ boot_openrel(char *relname)
 	TableScanDesc scan;
 	HeapTuple	tup;
 
+	/* Check that pg_class data is consistent now, rather than failing obscurely later */
+	

Re: In PG12, query with float calculations is slower than PG11

[Tom Lane]

Andres Freund  writes:
> I'd just rename the macro to the name of the inline function. No need to
> have a verbose change in all callsites just to update the name imo.

+1, that's what I had in mind too.  That does suggest though that we
ought to make sure the macro has single-eval behavior, so that you
don't need to know it's a macro.

regards, tom lane

Re: In PG12, query with float calculations is slower than PG11

[Andres Freund]

On 2020-02-12 17:49:14 +, Emre Hasegeli wrote:
> > Nor do I see how it's going to be ok to just rename the function in a
> > stable branch.
> 
> I'll post another version to keep them around.

I'd just rename the macro to the name of the inline function. No need to
have a verbose change in all callsites just to update the name imo.

Re: In PG12, query with float calculations is slower than PG11

[Emre Hasegeli]

> Wait, no. Didn't we get to the point that we figured out that the
> primary issue is the reversal of the order of what is checked is the
> primary problem, rather than the macro/inline piece?

Reversal of the order makes a small or no difference.  The
macro/inline change causes the real slowdown at least on GCC.

> Nor do I see how it's going to be ok to just rename the function in a
> stable branch.

I'll post another version to keep them around.

Re: Exposure related to GUC value of ssl_passphrase_command

[Fujii Masao]

On Fri, Nov 8, 2019 at 4:24 PM Amit Langote  wrote:
>
> Hello.
>
> On Tue, Nov 5, 2019 at 5:15 PM Moon, Insung  
> wrote:
> > Deal Hackers.
> >
> > The value of ssl_passphrase_command is set so that an external command
> > is called when the passphrase for decrypting an SSL file such as a
> > private key is obtained.
> > Therefore, easily set to work with echo "passphrase" or call to
> > another get of passphrase application.
> >
> > I think that this GUC value doesn't contain very sensitive data,
> > but just in case, it's dangerous to be visible to all users.
> > I think do not possible these cases, but if a used echo external
> > commands or another external command,  know what application used to
> > get the password, maybe we can't be convinced that there's the safety
> > of using abuse by backtracking on applications.
> > So I think to the need only superusers or users with the default role
> > of pg_read_all_settings should see these values.
> >
> > Patch is very simple.
> > How do you think about my thoughts like this?
>
> I'm hardly an expert on this topic, but reading this blog post about
> ssl_passphrase_command:
>
> https://www.2ndquadrant.com/en/blog/postgresql-passphrase-protected-ssl-keys-systemd/
>
> which mentions that some users might go with the very naive
> configuration such as:
>
> ssl_passphrase_command = 'echo "secret"'
>
> maybe it makes sense to protect its value from everyone but superusers.
>
> So +1.

Seems this proposal is reasonable.

Regards,

-- 
Fujii Masao

Re: Just for fun: Postgres 20?

[Christopher Browne]

On Wed, 12 Feb 2020 at 08:28, Alvaro Herrera 
wrote:

> marcelo zen escribió:
> > I'd rather have releases being made when the software is ready and not
> when
> > the calendar year mandates it.
> > It seems like a terrible idea.
>
> But we do actually release on calendar year.  While it seems not
> unreasonable that we might fail to ship in time, that would likely lead
> to one month, two months of delay.  Four months?  I don't think anybody
> even imagines such a long delay.  It would be seen as utter,
> unacceptable failure of our release team.
>

All said, I think there's some merit to avoiding a PostgreSQL 13 release,
because
there's enough superstition out there about the infamous "number 13."

Perhaps we could avert it by doing an "April Fool's Postgres 13" release?
-- 
When confronted by a difficult problem, solve it by reducing it to the
question, "How would the Lone Ranger handle this?"

Wait event that should be reported while waiting for WAL archiving to finish

[Fujii Masao]

Hi,

When I saw pg_stat_activity.wait_event while pg_basebackup -X none
is waiting for WAL archiving to finish, it was either NULL or
CheckpointDone. I think this is confusing. What about introducing
new wait_event like WAIT_EVENT_BACKUP_WAIT_WAL_ARCHIVE
(BackupWaitWalArchive) and reporting it during that period?

Regards,

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

Re: Global temporary tables

[Phil Florent]

Hi,



I am very interested in this feature that will conform to the SQL standard and 
I read that :



Session 1:

create global temp table gtt(x integer);

insert into gtt values (generate_series(1,10));



Session 2:

insert into gtt values (generate_series(1,20));



Session1:

create index on gtt(x);

explain select * from gtt where x = 1;



Session2:

explain select * from gtt where x = 1;

??? Should we use index here?



My answer is - yes.

Just because:

- Such behavior is compatible with regular tables. So it will not

confuse users and doesn't require some complex explanations.

- It is compatible with Oracle.



There is a confusion. Sadly it does not work like that at all with Oracle. 
Their implementation is buggy in my opinion.

Here is a very simple test case to prove it with the latest version (january 
2020) :



Connected to:

Oracle Database 19c Enterprise Edition Release 19.0.0.0.0 - 
Production

Version 19.6.0.0.0



-- session 1

create global temporary table gtt(x integer);

Table created.



-- session 2

insert into gtt SELECT level FROM dual CONNECT BY LEVEL <= 10;

10 rows created.



-- session 1

create index igtt on gtt(x);

Index created.



-- session 2

select * from gtt where x = 9;



no rows selected



select /*+ FULL(gtt) */ * from gtt where x = 9;



 X

--

 9



What happened ? The optimizer (planner) knows the new index igtt can be 
efficient via dynamic sampling. Hence, igtt is used at execution time...but it 
is NOT populated. By default I obtained no line. If I force a full scan of the 
table with a hint /*+ FULL */ you can see that I obtain my line 9. Different 
results with different exec plans it's a WRONG RESULT bug, the worst kind of 
bugs.

Please don't consider Oracle as a reference for your implementation. I am 100% 
sure you can implement and document that better than Oracle. E.g index is 
populated and considered only  for transactions that started after the index 
creation or something like that. It would be far better than this misleading 
behaviour.

Regards,

Phil








Télécharger Outlook pour Android


From: Konstantin Knizhnik 
Sent: Monday, February 10, 2020 5:48:29 PM
To: Tomas Vondra ; Philippe BEAUDOIN 

Cc: pgsql-hackers@lists.postgresql.org ; 
Konstantin Knizhnik 
Subject: Re: Global temporary tables


Sorry, small typo in the last patch.

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

Re: In PG12, query with float calculations is slower than PG11

[Andres Freund]

Hi,

On 2020-02-12 11:54:13 +, Emre Hasegeli wrote:
> From fb5052b869255ef9465b1de92e84b2fb66dd6eb3 Mon Sep 17 00:00:00 2001
> From: Emre Hasegeli 
> Date: Fri, 7 Feb 2020 10:27:25 +
> Subject: [PATCH] Bring back CHECKFLOATVAL() macro
> 
> The inline functions added by 6bf0bc842b caused the conditions of
> overflow/underflow checks to be evaluated when no overflow/underflow
> happen.  This slowed down floating point operations.  This commit brings
> back the macro that was in use before 6bf0bc842b to fix the performace
> regression.

Wait, no. Didn't we get to the point that we figured out that the
primary issue is the reversal of the order of what is checked is the
primary problem, rather than the macro/inline piece?

Nor do I see how it's going to be ok to just rename the function in a
stable branch.

Greetings,

Andres Freund

Re: Minor issues in .pgpass

[Fujii Masao]

On 2020/01/22 9:06, David Fetter wrote:

On Tue, Jan 21, 2020 at 03:27:50PM +0900, Fujii Masao wrote:

Hi,

When I was researching the maximum length of password in PostgreSQL
to answer the question from my customer, I found that there are two
minor issues in .pgpass file.

(1) If the length of a line in .pgpass file is larger than 319B,
libpq silently treats each 319B in the line as a separate
setting line.


This seems like a potentially serious bug. For example, a truncated
password could get retried enough times to raise intruder alarms, and
it wouldn't be easy to track down.


(2) The document explains that a line beginning with # is treated
as a comment in .pgpass. But as far as I read the code,
there is no code doing such special handling.


This is a flat-out bug, as it violates a promise the documentation has
made.


Also if the length of that "comment" line is larger than 319B,
the latter part of the line can be treated as valid setting.



You may think that these unexpected behaviors are not so harmful
in practice because "usually" the length of password setting line is
less than 319B and the hostname beginning with # is less likely to be
used. But the problem exists. And there are people who want to use
large password or to write a long comment (e.g., with multibyte
characters like Japanese) in .pgass, so these may be more harmful
in the near future.

For (1), I think that we should make libpq warn if the length of a line
is larger than 319B, and throw away the remaining part beginning from
320B position. Whether to enlarge the length of a line should be
a separate discussion, I think.


Agreed.


For (2), libpq should treat any lines beginning with # as comments.


Patch attached. This patch does the above (1) and (2).


Would it make sense for lines starting with whitespace and then # to
be treated as comments, too, e.g.:


Could you tell me why you want to treat such a line as comment?
Basically I don't want to change the existing rules for parsing
.pgpass file more thane necessary.

Regards,

--
Fujii Masao
NTT DATA CORPORATION
Advanced Platform Technology Group
Research and Development Headquarters
diff --git a/src/interfaces/libpq/fe-connect.c 
b/src/interfaces/libpq/fe-connect.c
index 408000af83..eb34d2122f 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -6949,6 +6949,7 @@ passwordFromFile(const char *hostname, const char *port, 
const char *dbname,
 {
FILE   *fp;
struct stat stat_buf;
+   int line_number = 0;
 
 #define LINELEN NAMEDATALEN*5
charbuf[LINELEN];
@@ -7014,10 +7015,42 @@ passwordFromFile(const char *hostname, const char 
*port, const char *dbname,
   *p1,
   *p2;
int len;
+   int buflen;
 
if (fgets(buf, sizeof(buf), fp) == NULL)
break;
 
+   line_number++;
+   buflen = strlen(buf);
+   if (buflen >= sizeof(buf) - 1 && buf[buflen - 1] != '\n')
+   {
+   chartmp[LINELEN];
+   int tmplen;
+
+   /*
+* Warn if this password setting line is too long,
+* because it's unexpectedly truncated.
+*/
+   if (buf[0] != '#')
+   fprintf(stderr,
+   libpq_gettext("WARNING: line %d 
too long in password file \"%s\"\n"),
+   line_number, pgpassfile);
+
+   /* eat rest of the line */
+   while (!feof(fp) && !ferror(fp))
+   {
+   if (fgets(tmp, sizeof(tmp), fp) == NULL)
+   break;
+   tmplen = strlen(tmp);
+   if (tmplen < sizeof(tmp) -1 || tmp[tmplen - 1] 
== '\n')
+   break;
+   }
+   }
+
+   /* ignore comments */
+   if (buf[0] == '#')
+   continue;
+
/* strip trailing newline and carriage return */
len = pg_strip_crlf(buf);
 

Bug in pg_restore with EventTrigger in parallel mode

[Fabrízio de Royes Mello]

Hi all,

Today digging into a customer issue about errors in pg_restore I realized
that pg_restore dispatch a worker to restore EventTrigger
during restore_toc_entries_parallel. IMHO EventTriggers should be restored
during the restore_toc_entries_postfork in serial mode.

For example this simple database schema:

BEGIN;

CREATE TABLE foo(c1 bigserial NOT NULL, c2 varchar(100) NOT NULL, PRIMARY
KEY (c1));
INSERT INTO foo (c2) SELECT 'Foo '||id FROM generate_series(0,10) id;
CREATE INDEX foo_1 ON foo (c2);

CREATE TABLE bar(c1 bigserial NOT NULL, c2 bigint REFERENCES public.foo, c3
varchar(100), PRIMARY KEY (c1));
INSERT INTO bar (c2, c3) SELECT (random()*10)::bigint+1, 'Bar '||id FROM
generate_series(1,1) id;
CREATE INDEX bar_1 ON bar (c2);
CREATE INDEX bar_2 ON bar (c3);

CREATE OR REPLACE FUNCTION f_test_ddl_trigger()
RETURNS event_trigger AS
$$
DECLARE
  r RECORD;
BEGIN
  FOR r IN
SELECT objid, objsubid, schema_name, objid::regclass::text AS
table_name, command_tag, object_type, object_identity
FROM pg_event_trigger_ddl_commands()
  LOOP
RAISE INFO 'RUN EVENT TRIGGER %', r;
  END LOOP;
END;
$$
LANGUAGE plpgsql;

CREATE EVENT TRIGGER test_ddl_trigger
ON ddl_command_end
EXECUTE PROCEDURE f_test_ddl_trigger();

COMMIT;

Running the dump:
$ bin/pg_dump -Fc -f /tmp/teste.dump fabrizio

Restoring with one worker everything is ok:
fabrizio@macanudo:~/pgsql
$ bin/pg_restore -Fc -d fabrizio_restore_serial /tmp/teste.dump | grep 'RUN
EVENT TRIGGER'

Running with more the one worker:
fabrizio@macanudo:~/pgsql
$ bin/pg_restore -Fc -j2 -d fabrizio_restore_parallel /tmp/teste.dump |
grep 'RUN EVENT TRIGGER'
pg_restore: INFO:  RUN EVENT TRIGGER (16906,0,public,public.bar,"ALTER
TABLE",table,public.bar)

In parallel mode it's firing the EventTrigger and it can't be happen.
Poking around it I did some test with attached just to leave EventTriggers
in pending_list to process it in restore_toc_entries_postfork and
everything is ok, but my solution is very ugly, so maybe we need to invent
a new RestorePass to take care of it like RESTORE_PASS_ACL and
RESTORE_PASS_REFRESH. I can provide a more polished patch if it'll be a
good way to do that.

Regards,

-- 
   Fabrízio de Royes Mello Timbira - http://www.timbira.com.br/
   PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento
diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
index 77bf9edaba..9762e4c973 100644
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -4357,7 +4357,8 @@ move_to_ready_list(TocEntry *pending_list,
 		next_te = te->pending_next;
 
 		if (te->depCount == 0 &&
-			_tocEntryRestorePass(te) == pass)
+			_tocEntryRestorePass(te) == pass &&
+			strcmp(te->desc, "EVENT TRIGGER") != 0) /* leave EVENT TRIGGER in pending_list */
 		{
 			/* Remove it from pending_list ... */
 			pending_list_remove(te);

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

[Tom Lane]

Amit Kapila  writes:
> On Wed, Feb 12, 2020 at 7:36 AM Masahiko Sawada
>  wrote:
>> On Wed, 12 Feb 2020 at 00:43, Tom Lane  wrote:
>>> I would like to suggest that we do something similar to Robert Haas'
>>> excellent hack (daa7527af) for the !HAVE_SPINLOCK case in lmgr/spin.c,

>> My original proposal used LWLocks and hash tables for relation
>> extension but there was a discussion that using LWLocks is not good
>> because it's not interruptible[1].

> Hmm, but we use LWLocks for (a) WALWrite/Flush (see the usage of
> WALWriteLock), (b) writing the shared buffer contents (see
> io_in_progress lock and its usage in FlushBuffer) and might be for few
> other similar stuff.  Many times those take more time than extending a
> block in relation especially when we combine the WAL write for
> multiple commits.  So, if this is a problem for relation extension
> lock, then the same thing holds true there also.

Yeah.  I would say a couple more things:

* I see no reason to think that a relation extension lock would ever
be held long enough for noninterruptibility to be a real issue.  Our
expectations for query cancel response time are in the tens to
hundreds of msec anyway.

* There are other places where an LWLock can be held for a *long* time,
notably the CheckpointLock.  If we do think this is an issue, we could
devise a way to not insist on noninterruptibility.  The easiest fix
is just to do a matching RESUME_INTERRUPTS after getting the lock and
HOLD_INTERRUPTS again before releasing it; though maybe it'd be worth
offering some slightly cleaner way.  Point here is that LWLockAcquire
only does that because it's useful to the majority of callers, not
because it's graven in stone that it must be like that.

In general, if we think there are issues with LWLock, it seems to me
we'd be better off to try to fix them, not to invent a whole new
single-purpose lock manager that we'll have to debug and maintain.
I do not see anything about this problem that suggests that that would
provide a major win.  As Andres has noted, there are lots of other
aspects of it that are likely to be more useful to spend effort on.

regards, tom lane

Re: Getting rid of some more lseek() calls

[Alvaro Herrera]

On 2020-Feb-12, Thomas Munro wrote:

> Hmm.  Well, on Unix we have to choose between "tell me the size but
> also change the position that I either don't care about or have to
> undo", and "tell me the size but also tell me all this other stuff I
> don't care about".  Since Windows apparently has GetFileSizeEx(), why
> not use that when that's exactly what you want?  It apparently
> understands large files.

I was already thinking that it might be better to make the new function
just "tell me the file size" without leaking the details of *how* we do
it, before reading about this Windows call.  That reinforces it, IMO.

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

Re: Just for fun: Postgres 20?

[Juan José Santamaría Flecha]

On Wed, Feb 12, 2020 at 3:47 PM Tom Lane  wrote:

>
> Yeah; I don't think it's *that* unlikely for it to happen again.  But
> my own principal concern about this mirrors what somebody else already
> pointed out: the one-major-release-per-year schedule is not engraved on
> any stone tablets.  So I don't want to go to a release numbering system
> that depends on us doing it that way for the rest of time.
>
>
We could you use  as version identifier, so people will not expect
correlative numbering. SQL Server is being released every couple of years
and they are using this naming shema. The problem would be releasing twice
the same year, but how likely would that be?

Regards,

Juan José Santamaría Flecha

Re: Just for fun: Postgres 20?

[Alvaro Herrera]

Andreas Karlsson escribió:
> On 2/12/20 12:07 AM, Alvaro Herrera wrote:
> > marcelo zen escribió:
> > > I'd rather have releases being made when the software is ready and not 
> > > when
> > > the calendar year mandates it.
> > > It seems like a terrible idea.
> > 
> > But we do actually release on calendar year.  While it seems not
> > unreasonable that we might fail to ship in time, that would likely lead
> > to one month, two months of delay.  Four months?  I don't think anybody
> > even imagines such a long delay.  It would be seen as utter,
> > unacceptable failure of our release team.
> 
> It has actually happened once: PostgreSQL 9.5 was released in 2016-01-07.

We didn't have a formal release team back then :-)  It started with 9.6.
Some history: https://wiki.postgresql.org/wiki/RMT  Anyway, I concede
that it's too recent history to say that this will never happen again.

Retroactively we could still have named "Postgres 15" the one released
on January 2016.  It was clearly the development line made during 2015,
it just got a little bit delayed.

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

Updating row and width estimates in postgres_fdw

[Ashutosh Bapat]

Hi,
In postgresGetForeignJoinPaths(), I see

   /* Estimate costs for bare join relation */
estimate_path_cost_size(root, joinrel, NIL, NIL, NULL,
, , _cost, _cost);
/* Now update this information in the joinrel */
joinrel->rows = rows;
joinrel->reltarget->width = width;

This code is good as well as bad.

For a join relation, we estimate the number of rows in
set_joinrel_size_estimates() inside build_*_join_rel() and set the width of
the join when building the targetlist. For foreign join, the size estimates
may not be correct but width estimate should be. So updating the number of
rows looks good since it would be better than what
set_joinrel_size_etimates() might come up with but here are the problems
with this code
1. The rows estimated by estimate_path_cost_size() are better only when
use_remote_estimates is true. So, we should be doing this only when
use_remote_estimate is true.
2. This function gets called after local paths for the first pair for this
join have been added. So those paths are not being judged fairly and
perhaps we might be throwing away better paths just because the local
estimates with which they were created were very different from the remote
estimates.

A better way would be to get the estimates and setup fpinfo for a joinrel
in build_join_rel() and later add paths similar to what we do for base
relations. That means we split the current hook GetForeignJoinPaths into
two - one to get estimates and the other to setup fpinfo.

Comments?
--
Best Wishes,
Ashutosh Bapat

Re: Memory-comparable Serialization of Data Types

[Shichao Jin]

Thank you for both your feedback. Yes, as indicated by Peter, we indeed use
that technique in comparison in index, and now we will try passing
comparator to the storage engine according to Alvaro's suggestion.

Best,
Shichao




On Tue, 11 Feb 2020 at 17:16, Peter Geoghegan  wrote:

> On Tue, Feb 11, 2020 at 1:40 PM Alvaro Herrera 
> wrote:
> > I think adding that would be too much of a burden, both for the project
> > itself as for third-party type definitions; I think we'd rather rely on
> > calling the BTORDER_PROC btree support function for the type.
>
> An operator class would still need to provide a BTORDER_PROC. What I
> describe would be an optional capability. This is something that I
> have referred to as key normalization in the past:
>
> https://wiki.postgresql.org/wiki/Key_normalization
>
> I think that it would only make sense as an enabler of multiple
> optimizations -- not just the memcmp()/strcmp() thing. A common
> strcmp()'able binary string format can be used in many different ways.
> Note that this has nothing to do with changing the representation used
> by the vast majority of all tuples -- just the pivot tuples, which are
> mostly located in internal pages. They only make up less than 1% of
> all pages in almost all cases.
>
> I intend to prototype this technique within the next year. It's
> possible that it isn't worth the trouble, but there is only one way to
> find out. I might just work on the "abbreviated keys in internal
> pages" thing, for example. Though you really need some kind of prefix
> compression to make that effective.
>
> --
> Peter Geoghegan
>


-- 
Shichao Jin
PhD Student at University of Waterloo, Canada
e-mail: jsc0...@gmail.com
homepage: http://sites.google.com/site/csshichaojin/

Support external parameters in EXECUTE command

[Peter Eisentraut]

Continuing the discussion in [0], here is a patch that allows parameter 
references in the arguments of the EXECUTE command.  The main purpose is 
submitting protocol-level parameters, but the added regression test case 
shows another way to exercise it.


What's confusing is that the code already contains a reference that 
indicates that this should be possible:


/* Evaluate parameters, if any */
if (entry->plansource->num_params > 0)
{
/*
 * Need an EState to evaluate parameters; must not delete it 
till end
 * of query, in case parameters are pass-by-reference.  Note 
that the

 * passed-in "params" could possibly be referenced in the parameter
 * expressions.
 */
estate = CreateExecutorState();
estate->es_param_list_info = params;
paramLI = EvaluateParams(pstate, entry, stmt->params, estate);
}

I'm not sure what this is supposed to do without my patch on top of it. 
If I remove the estate->es_param_list_info assignment, no tests fail 
(except the one I added).  Either this is a leftover from previous 
variants of this code (as discussed in [0]), or there is something I 
haven't understood.



[0]: 
https://www.postgresql.org/message-id/flat/6e7aa4a1-be6a-1a75-b1f9-83a678e5184a%402ndquadrant.com


--
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From 9c1ba747f5c8b424ca8a1e4c0feb4c859312203e Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Tue, 11 Feb 2020 19:55:25 +0100
Subject: [PATCH v1] Support external parameters in EXECUTE command

This allows the arguments of the EXECUTE command to be parameters
themselves, for example passed as separate parameters on the protocol
level.
---
 src/backend/commands/prepare.c| 40 +++
 src/test/regress/expected/prepare.out | 23 +++
 src/test/regress/sql/prepare.sql  | 21 ++
 3 files changed, 84 insertions(+)

diff --git a/src/backend/commands/prepare.c b/src/backend/commands/prepare.c
index c4e4b6eaec..543de3493d 100644
--- a/src/backend/commands/prepare.c
+++ b/src/backend/commands/prepare.c
@@ -33,6 +33,7 @@
 #include "tcop/pquery.h"
 #include "tcop/utility.h"
 #include "utils/builtins.h"
+#include "utils/lsyscache.h"
 #include "utils/snapmgr.h"
 #include "utils/timestamp.h"
 
@@ -175,6 +176,42 @@ PrepareQuery(ParseState *pstate, PrepareStmt *stmt,
   true);
 }
 
+/*
+ * Parser callback for resolving parameter references from an existing
+ * ParamListInfo structure.
+ */
+static Node *
+pli_paramref_hook(ParseState *pstate, ParamRef *pref)
+{
+   ParamListInfo paramInfo = (ParamListInfo) pstate->p_ref_hook_state;
+   int paramno = pref->number;
+   ParamExternData *ped;
+   ParamExternData pedws;
+   Param  *param;
+
+   /* Check parameter number is valid */
+   if (paramno <= 0 || paramno > paramInfo->numParams)
+   ereport(ERROR,
+   (errcode(ERRCODE_UNDEFINED_PARAMETER),
+errmsg("there is no parameter $%d", paramno),
+parser_errposition(pstate, pref->location)));
+
+   if (paramInfo->paramFetch != NULL)
+   ped = paramInfo->paramFetch(paramInfo, paramno, false, );
+   else
+   ped = >params[paramno - 1];
+
+   param = makeNode(Param);
+   param->paramkind = PARAM_EXTERN;
+   param->paramid = paramno;
+   param->paramtype = ped->ptype;
+   param->paramtypmod = -1;
+   param->paramcollid = get_typcollation(param->paramtype);
+   param->location = pref->location;
+
+   return (Node *) param;
+}
+
 /*
  * ExecuteQuery --- implement the 'EXECUTE' utility statement.
  *
@@ -199,6 +236,9 @@ ExecuteQuery(ParseState *pstate,
int eflags;
longcount;
 
+   pstate->p_ref_hook_state = (void *) params;
+   pstate->p_paramref_hook = pli_paramref_hook;
+
/* Look it up in the hash table */
entry = FetchPreparedStatement(stmt->name, true);
 
diff --git a/src/test/regress/expected/prepare.out 
b/src/test/regress/expected/prepare.out
index 3306c696b1..a0fec13c6b 100644
--- a/src/test/regress/expected/prepare.out
+++ b/src/test/regress/expected/prepare.out
@@ -187,3 +187,26 @@ SELECT name, statement, parameter_types FROM 
pg_prepared_statements
 --+---+-
 (0 rows)
 
+-- check parameter handling
+CREATE TABLE t1 (a int);
+PREPARE p1 AS INSERT INTO t1 (a) VALUES ($1);
+CREATE FUNCTION f1(x int) RETURNS int
+LANGUAGE SQL
+AS $$
+EXECUTE p1($1);
+SELECT null::int;
+$$;
+SELECT f1(2);
+ f1 
+
+   
+(1 row)
+
+SELECT * FROM t1;
+ a 
+---
+ 2
+(1 row)
+
+DROP FUNCTION f1(int);
+DROP TABLE t1;
diff --git a/src/test/regress/sql/prepare.sql b/src/test/regress/sql/prepare.sql
index 

Re: Just for fun: Postgres 20?

[Tom Lane]

Andreas Karlsson  writes:
> On 2/12/20 12:07 AM, Alvaro Herrera wrote:
>> But we do actually release on calendar year.  While it seems not
>> unreasonable that we might fail to ship in time, that would likely lead
>> to one month, two months of delay.  Four months?  I don't think anybody
>> even imagines such a long delay.  It would be seen as utter,
>> unacceptable failure of our release team.

> It has actually happened once: PostgreSQL 9.5 was released in 2016-01-07.

Yeah; I don't think it's *that* unlikely for it to happen again.  But
my own principal concern about this mirrors what somebody else already
pointed out: the one-major-release-per-year schedule is not engraved on
any stone tablets.  So I don't want to go to a release numbering system
that depends on us doing it that way for the rest of time.

regards, tom lane

Re: Just for fun: Postgres 20?

[Andreas Karlsson]

On 2/12/20 12:07 AM, Alvaro Herrera wrote:

marcelo zen escribió:

I'd rather have releases being made when the software is ready and not when
the calendar year mandates it.
It seems like a terrible idea.


But we do actually release on calendar year.  While it seems not
unreasonable that we might fail to ship in time, that would likely lead
to one month, two months of delay.  Four months?  I don't think anybody
even imagines such a long delay.  It would be seen as utter,
unacceptable failure of our release team.


It has actually happened once: PostgreSQL 9.5 was released in 2016-01-07.


Others have commented in this thread that the idea seems ridiculous, and
I concur.  But the reason is not what you say.  The reason, I think, is
that for years we spent months each time debating what to name the next
release; and only recently, in version 10, we decided to change our
numbering scheme so that these pointless discussions are gone for good.
To think that just three years after that we're going to waste months
again discussing the same topic ...?  Surely not.


Agreed, and personally I do not see enough benefit from moving to 20.X 
or 2020.X for it to be worth re-opening this discussion. The bikeshed is 
already painted.


Andreas

Re: Just for fun: Postgres 20?

[Alvaro Herrera]

marcelo zen escribió:
> I'd rather have releases being made when the software is ready and not when
> the calendar year mandates it.
> It seems like a terrible idea.

But we do actually release on calendar year.  While it seems not
unreasonable that we might fail to ship in time, that would likely lead
to one month, two months of delay.  Four months?  I don't think anybody
even imagines such a long delay.  It would be seen as utter,
unacceptable failure of our release team.

Others have commented in this thread that the idea seems ridiculous, and
I concur.  But the reason is not what you say.  The reason, I think, is
that for years we spent months each time debating what to name the next
release; and only recently, in version 10, we decided to change our
numbering scheme so that these pointless discussions are gone for good.
To think that just three years after that we're going to waste months
again discussing the same topic ...?  Surely not.

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

Re: Add PostgreSQL home page to --help output

[Daniel Gustafsson]

> On 12 Feb 2020, at 11:54, Peter Eisentraut  
> wrote:
> 
> On 2020-02-11 10:34, Daniel Gustafsson wrote:
>> Pardon my weak autoconf-skills, what does the inverted brackets (]foo[ as
>> opposed to [foo]) do in the below?
>> -Please also contact  to see about
>> +Please also contact <]AC_PACKAGE_BUGREPORT[> to see about
> 
> AC_PACKAGE_BUGREPORT is an Autoconf macro, set up by AC_INIT.  The call above 
> is in the context of
> 
> AC_MSG_ERROR([[ ... text ... ]])
> 
> The brackets are quote characters that prevent accidentally expanding a token 
> in the text as a macro.  So in order to get AC_PACKAGE_BUGREPORT expanded, we 
> need to undo one level of quoting.
> 
> See also 
> 
>  for more information.

Aha, that's what I was looking for in the docs but didn't find.  Thanks for
sharing!

cheers ./daniel

Re: In PG12, query with float calculations is slower than PG11

[Emre Hasegeli]

> Should we update the same macro in contrib/btree_gist/btree_utils_num.h too?

I posted another version incorporating this.

Re: In PG12, query with float calculations is slower than PG11

[Emre Hasegeli]

> But the comment does not explain that this test has to be in that
> order, or the compiler will for non-constant arguments evalute
> the (now) right-side first. E.g. if I understand this correctly:
>
>   +  if (!(zero_is_valid) && unlikely((val) == 0.0)
>
> would have the same problem of evaluating "zero_is_valid" (which
> might be an isinf(arg1) || isinf(arg2)) first and so be the same thing
> we try to avoid with the macro? Maybe adding this bit of info to the
> comment makes it clearer?

Added.

> Also, a few places use the macro as:
>
>   + CHECKFLOATVAL(result, true, true);
>
> which evaluates to a complete NOP in both cases. IMHO this could be
> replaced with a comment like:
>
>   + // No CHECKFLOATVAL() needed, as both inf and 0.0 are valid
>
> (or something along the lines of "no error can occur"), as otherwise
> CHECKFLOATVAL() implies to the casual reader that there are some checks
> done, while in reality no real checks are done at all (and hopefully
> the compiler optimizes everything away, which might not be true for
> debug builds).

I don't know why those trigonometric functions don't check for
overflow/underflow like all the rest of float.c.  I'll submit another
patch to make them error when overflow/underflow.

The new version is attached.
From fb5052b869255ef9465b1de92e84b2fb66dd6eb3 Mon Sep 17 00:00:00 2001
From: Emre Hasegeli 
Date: Fri, 7 Feb 2020 10:27:25 +
Subject: [PATCH] Bring back CHECKFLOATVAL() macro

The inline functions added by 6bf0bc842b caused the conditions of
overflow/underflow checks to be evaluated when no overflow/underflow
happen.  This slowed down floating point operations.  This commit brings
back the macro that was in use before 6bf0bc842b to fix the performace
regression.

Reported-by: Keisuke Kuroda 
Author: Keisuke Kuroda 
Discussion: https://www.postgresql.org/message-id/CANDwggLe1Gc1OrRqvPfGE%3DkM9K0FSfia0hbeFCEmwabhLz95AA%40mail.gmail.com
---
 contrib/btree_gist/btree_utils_num.h |  6 +--
 src/backend/utils/adt/float.c| 66 
 src/backend/utils/adt/geo_ops.c  |  2 +-
 src/include/utils/float.h| 76 
 4 files changed, 70 insertions(+), 80 deletions(-)

diff --git a/contrib/btree_gist/btree_utils_num.h b/contrib/btree_gist/btree_utils_num.h
index 1fedfbe82d..a3227fd758 100644
--- a/contrib/btree_gist/btree_utils_num.h
+++ b/contrib/btree_gist/btree_utils_num.h
@@ -84,30 +84,30 @@ typedef struct
  */
 #define INTERVAL_TO_SEC(ivp) \
 	(((double) (ivp)->time) / ((double) USECS_PER_SEC) + \
 	 (ivp)->day * (24.0 * SECS_PER_HOUR) + \
 	 (ivp)->month * (30.0 * SECS_PER_DAY))
 
 #define GET_FLOAT_DISTANCE(t, arg1, arg2)	Abs( ((float8) *((const t *) (arg1))) - ((float8) *((const t *) (arg2))) )
 
 /*
  * check to see if a float4/8 val has underflowed or overflowed
- * borrowed from src/backend/utils/adt/float.c
+ * borrowed from src/include/utils/float.c
  */
 #define CHECKFLOATVAL(val, inf_is_valid, zero_is_valid)			\
 do {			\
-	if (isinf(val) && !(inf_is_valid))			\
+	if (unlikely(isinf(val) && !(inf_is_valid)))\
 		ereport(ERROR,			\
 (errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),	\
 		  errmsg("value out of range: overflow")));\
 \
-	if ((val) == 0.0 && !(zero_is_valid))		\
+	if (unlikely((val) == 0.0 && !(zero_is_valid)))\
 		ereport(ERROR,			\
 (errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),	\
 		 errmsg("value out of range: underflow")));\
 } while(0)
 
 
 extern Interval *abs_interval(Interval *a);
 
 extern bool gbt_num_consistent(const GBT_NUMKEY_R *key, const void *query,
 			   const StrategyNumber *strategy, bool is_leaf,
diff --git a/src/backend/utils/adt/float.c b/src/backend/utils/adt/float.c
index a90d4db215..5885719850 100644
--- a/src/backend/utils/adt/float.c
+++ b/src/backend/utils/adt/float.c
@@ -1184,21 +1184,21 @@ ftod(PG_FUNCTION_ARGS)
 
 
 /*
  *		dtof			- converts a float8 number to a float4 number
  */
 Datum
 dtof(PG_FUNCTION_ARGS)
 {
 	float8		num = PG_GETARG_FLOAT8(0);
 
-	check_float4_val((float4) num, isinf(num), num == 0);
+	CHECKFLOATVAL((float4) num, isinf(num), num == 0);
 
 	PG_RETURN_FLOAT4((float4) num);
 }
 
 
 /*
  *		dtoi4			- converts a float8 number to an int4 number
  */
 Datum
 dtoi4(PG_FUNCTION_ARGS)
@@ -1438,36 +1438,36 @@ dsqrt(PG_FUNCTION_ARGS)
 	float8		arg1 = PG_GETARG_FLOAT8(0);
 	float8		result;
 
 	if (arg1 < 0)
 		ereport(ERROR,
 (errcode(ERRCODE_INVALID_ARGUMENT_FOR_POWER_FUNCTION),
  errmsg("cannot take square root of a negative number")));
 
 	result = sqrt(arg1);
 
-	check_float8_val(result, isinf(arg1), arg1 == 0);
+	CHECKFLOATVAL(result, isinf(arg1), arg1 == 0);
 	PG_RETURN_FLOAT8(result);
 }
 
 
 /*
  *		dcbrt			- returns cube root of arg1
  */
 Datum
 dcbrt(PG_FUNCTION_ARGS)
 {
 	float8		arg1 = PG_GETARG_FLOAT8(0);
 	float8		result;
 
 	result = cbrt(arg1);
-	check_float8_val(result, isinf(arg1), arg1 == 0);
+	

Re: Add PostgreSQL home page to --help output

[Peter Eisentraut]

On 2020-02-11 10:34, Daniel Gustafsson wrote:

Pardon my weak autoconf-skills, what does the inverted brackets (]foo[ as
opposed to [foo]) do in the below?

-Please also contact  to see about
+Please also contact <]AC_PACKAGE_BUGREPORT[> to see about


AC_PACKAGE_BUGREPORT is an Autoconf macro, set up by AC_INIT.  The call 
above is in the context of


AC_MSG_ERROR([[ ... text ... ]])

The brackets are quote characters that prevent accidentally expanding a 
token in the text as a macro.  So in order to get AC_PACKAGE_BUGREPORT 
expanded, we need to undo one level of quoting.


See also 
 
for more information.


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

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

[Amit Kapila]

On Wed, Feb 12, 2020 at 10:24 AM Andres Freund  wrote:
>
> Hi,
>
> On 2020-02-11 08:01:34 +0530, Amit Kapila wrote:
> > I don't see much downside with the patch, rather there is a
> > performance increase of 3-9% in various scenarios.
>
> As I wrote in [1] I started to look at this patch. My problem with itis
> that it just seems like the wrong direction architecturally to
> me. There's two main aspects to this:
>
> 1) It basically builds a another, more lightweight but less capable, of
>a lock manager that can lock more objects than we can have distinct
>locks for.  It is faster because it uses *one* hashtable without
>conflict handling, because it has fewer lock modes, and because it
>doesn't support detecting deadlocks. And probably some other things.
>
> 2) A lot of the contention around file extension comes from us doing
>multiple expensive things under one lock (determining current
>relation size, searching victim buffer, extending file), and in tiny
>increments (growing a 1TB table by 8kb). This patch doesn't address
>that at all.
>

It seems to me both the two points try to address the performance
angle of the patch, but here our actual intention was to make this
lock block among parallel workers so that we can implement/improve
some of the parallel writes operations (like parallelly vacuuming the
heap or index, parallel bulk load, etc.).  Both independently are
worth accomplishing, but not w.r.t parallel writes.  Here, we were
doing some benchmarking to see if we haven't regressed performance in
any cases.

> I've focused on 1) in the email referenced above ([1]). Here I'll focus
> on 2).
>
>
>
> Which, I think, pretty clearly shows a few things:
>

I agree with all your below observations.

> 1) It's crucial to move acquiring a victim buffer to the outside of the
>extension lock, as for copy acquiring the victim buffer will commonly
>cause a buffer having to be written out, due to the ringbuffer. This
>is even more crucial when using a logged table, as the writeout then
>also will often also trigger a WAL flush.
>
>While doing so will sometimes add a round of acquiring the buffer
>mapping locks, having to do the FlushBuffer while holding the
>extension lock is a huge problem.
>
>This'd also move a good bit of the cost of finding (i.e. clock sweep
>/ ringbuffer replacement) and invalidating the old buffer mapping out
>of the lock.
>

I think this mostly because of the way currently code is arranged to
extend a block via ReadBuffer* API.  IIUC, currently the main
operations under relation extension lock are as follows:
a. get the block number for extension via smgrnblocks.
b. find victim buffer
c. associate buffer with the block no. found in step-a.
d. initialize the block with zeros
e. write the block
f.  PageInit

I think if we can rearrange such that steps b and c can be done after
e or f, then we don't need to hold the extension lock to find the
victim buffer.

> 2) We need to make the smgrwrite more efficient, it is costing a lot of
>time. A small additional experiment shows the cost of doing 8kb
>writes:
>
>I wrote a small program that just iteratively writes a 32GB file:
>
..
>
>
>So it looks like extending the file with posix_fallocate() might be a
>winner, but only if we actually can do so in larger chunks than 8kb
>at once.
>

A good experiment and sounds like worth doing.

>
>
> 3) We should move the PageInit() that's currently done with the
>extension lock held, to the outside. Since we get the buffer with
>RBM_ZERO_AND_LOCK these days, that should be safe.  Also, we don't
>need to zero the entire buffer both in RelationGetBufferForTuple()'s
>PageInit(), and in ReadBuffer_common() before calling smgrextend().
>

Agreed.

I feel all three are independent improvements and can be done separately.

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

Re: Yet another vectorized engine

[Hubert Zhang]

On Tue, Feb 11, 2020 at 1:20 AM Konstantin Knizhnik <
k.knizh...@postgrespro.ru> wrote:

>
> So looks like PG-13 provides significant advantages in OLAP queries
> comparing with 9.6!
> Definitely it doesn't mean that vectorized executor is not needed for new
> version of Postgres.
> Once been ported, I expect that it should provide comparable  improvement
> of performance.
>
> But in any case I think that vectorized executor makes sense only been
> combine with columnar store.
>

Thanks for the test. +1 on vectorize should be combine with columnar store.
I think when we support this extension
on master, we could try the new zedstore.
I'm not active on this work now, but will continue when I have time. Feel
free to join bring vops's feature into this extension.

Thanks

Hubert Zhang

RE: [Proposal] Add accumulated statistics for wait event

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

On Wed, Feb 12, 2020 at 5:42 AM, Craig Ringer wrote:
> > It seems performance difference is big in case of read only tests. The 
> > reason is that write time is relatively longer than the
> > processing time of the logic I added in the patch.
> 
> That's going to be a pretty difficult performance hit to justify.
> 
> Can we buffer collected wait events locally and spit the buffer to the
> stats collector at convenient moments? We can use a limited buffer
> size with an overflow flag, so we degrade the results rather than
> falling over or forcing excessive stats reporting at inappropriate
times.

IIUC, currently each backend collects wait events locally. When each
backend goes to idle (telling the frontend that it is ready-for-query), it
reports wait event statistics to the stats collector. The interval of each
report is over than PGSTAT_STAT_INTERVAL(default 500ms). Also when
each backend exits, it also does report.

So if we do the read only test with 50 clients, each 50 backend reports
wait events statistics to the stats collector for almost every 500ms. If
that causes performance degradation, we can improve performance by
letting backends to report its statistics, for example, only at the
backend's exit.

(I think I can easily test this by building postgres with setting
PGSTAT_STAT_INTERVAL to a large value >> 500ms.)

> I suggest that this is also a good opportunity to add some more
> tracepoints to PostgreSQL. The wait events facilities are not very
> traceable right now.

Does that mean we will add TRACE_POSTGRESQL_ to every before/after
pgstat_report_wait_start?

> That way we have a zero-overhead-when-unused option that can also be
> used to aggregate the information per-query, per-user, etc.

I see. In that way, we can accomplish no overhead when DTrace is not
enabled and what we can measure is more customizable.

It is also curious how will overhead be if we implement wait events
statistics on DTrace scripts though I can't imagine how it will be because
I haven't used DTrace.


--
Yoshikazu Imai

Re: Print physical file path when checksum check fails

[Hubert Zhang]

Thanks Andres,

On Tue, Feb 11, 2020 at 5:30 AM Andres Freund  wrote:

> HHi,
>
> On 2020-02-10 16:04:21 +0800, Hubert Zhang wrote:
> > Currently we only print block number and relation path when checksum
> check
> > fails. See example below:
> >
> > ERROR: invalid page in block 333571 of relation base/65959/656195
>
> > DBA complains that she needs additional work to calculate which physical
> > file is broken, since one physical file can only contain `RELSEG_SIZE`
> > number of blocks. For large tables, we need to use many physical files
> with
> > additional suffix, e.g. 656195.1, 656195.2 ...
> >
> > Is that a good idea to also print the physical file path in error
> message?
> > Like below:
> >
> > ERROR: invalid page in block 333571 of relation base/65959/656195, file
> > path base/65959/656195.2
>
> I think that'd be a nice improvement. But:
>
> I don't think the way you did it is right architecturally. The
> segmenting is really something that lives within md.c, and we shouldn't
> further expose it outside of that. And e.g. the undo patchset uses files
> with different segmentation - but still goes through bufmgr.c.
>
> I wonder if this partially signals that the checksum verification piece
> is architecturally done in the wrong place currently? It's imo not good
> that every place doing an smgrread() needs to separately verify
> checksums. OTOH, it doesn't really belong inside smgr.c.
>
>
> This layering issue was also encountered in
>
> https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=3eb77eba5a51780d5cf52cd66a9844cd4d26feb0
> so perhaps we should work to reuse the FileTag it introduces to
> represent segments, without hardcoding the specific segment size?
>
>
I checked the FileTag commit. It calls `register_xxx_segment` inside md.c
to store the sync request into a hashtable and used by checkpointer later.

Checksum verify is simpler. We could move the `PageIsVerified` into md.c
(mdread). But we can not elog error inside md.c because read buffer mode
RBM_ZERO_ON_ERROR is at bugmgr.c level.

One idea is to change save the error message(or the FileTag) at (e.g. a
static string in bufmgr.c) by calling `register_checksum_failure` inside
mdread in md.c.

As for your concern about the need to do checksum verify after every
smgrread, we now move the checksum verify logic into md.c, but we still
need to check the checksum verify result after smgrread and reset buffer to
zero if mode is RBM_ZERO_ON_ERROR.

If this idea is OK, I will submit the new PR.

Thanks

Hubert Zhang

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

[Amit Kapila]

On Wed, Feb 12, 2020 at 7:36 AM Masahiko Sawada
 wrote:
>
> On Wed, 12 Feb 2020 at 00:43, Tom Lane  wrote:
> >
> > I took a brief look through this patch.  I agree with the fundamental
> > idea that we shouldn't need to use the heavyweight lock manager for
> > relation extension, since deadlock is not a concern and no backend
> > should ever need to hold more than one such lock at once.  But it feels
> > to me like this particular solution is rather seriously overengineered.
> > I would like to suggest that we do something similar to Robert Haas'
> > excellent hack (daa7527af) for the !HAVE_SPINLOCK case in lmgr/spin.c,
> > that is,
> >
> > * Create some predetermined number N of LWLocks for relation extension.
>
> My original proposal used LWLocks and hash tables for relation
> extension but there was a discussion that using LWLocks is not good
> because it's not interruptible[1]. Because of this reason and that we
> don't need to have two lock level (shared, exclusive) for relation
> extension lock we ended up with implementing dedicated lock manager
> for extension lock. I think we will have that problem if we use LWLocks.
>

Hmm, but we use LWLocks for (a) WALWrite/Flush (see the usage of
WALWriteLock), (b) writing the shared buffer contents (see
io_in_progress lock and its usage in FlushBuffer) and might be for few
other similar stuff.  Many times those take more time than extending a
block in relation especially when we combine the WAL write for
multiple commits.  So, if this is a problem for relation extension
lock, then the same thing holds true there also.  Now, there are cases
like when we extend the relation with multiple blocks, finding victim
buffer under this lock, etc. where this can be also equally or more
costly, but I think we can improve some of those cases (some of this
is even pointed by Andres in his email) if we agree on a fundamental
idea of using LWLocks as proposed by Tom.   I am not telling that we
implement Tom's idea without weighing its pros and cons, but it has an
appeal due to its simplicity.

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

Handing off SLRU fsyncs to the checkpointer

[Thomas Munro]

Hi,

In commit 3eb77eba we made it possible for any subsystem that wants a
file to be flushed as part of the next checkpoint to ask the
checkpointer to do that, as previously only md.c could do.

In the past, foreground CLOG flush stalls were a problem, but then
commit 33aaa139 cranked up the number of buffers, and then commit
5364b357 cranked it right up until the flushes mostly disappeared from
some benchmark workload but not so high that the resulting linear
searches through the buffer array destroyed the gains.  I know there
is interest in moving that stuff into regular shared buffers, so it
can be found via the buffer mapping system (and improve as that
improves), written back by the background writer (and improve as that
improves), managed with a proper replacement algorithm (and improve as
that improves), etc etc.  That sounds like a great idea to me, but
it's a big project.

In the meantime, one thing we could do is hand off the fsyncs, but I'm
not sure if it's still considered a real problem in the field given
the new parameters.

Anyway, I had a patch for that, that I used while testing commit
3eb77eba.  While reading old threads about SLRU today I found that
several people had wished for a thing exactly like that, so I dusted
it off and rebased it.


0001-Use-the-checkpointer-to-fsync-SLRU-files.patch
Description: Binary data

Re: Unix-domain socket support on Windows

[Peter Eisentraut]

Here is another patch set to enable this functionality.

0001 enables Unix-domain sockets on Windows, but leaves them turned off 
by default at run time, using the mechanism introduced by a9cff89f7e. 
This is relatively straightforward, except perhaps some aesthetic 
questions about how these different configuration bits are distributed 
around the various files.


0002 deals with pg_upgrade.  It preserves the existing behavior of not 
using Unix-domain sockets on Windows.  This could perhaps be enhanced 
later by either adding a command-line option or a run-time test.  It's 
too complicated right now.


0003 deals with how initdb should initialize postgresql.conf and 
pg_hba.conf.  It introduces a run-time test similar to how we detect 
presence of IPv6.  After I wrote this patch, I have come to think that 
this is overkill and we should just always leave the "local" line in 
pg_hba.conf even if there is no run-time support in the OS.  (I think 
the reason we do the run-time test for IPv6 is that we need it to parse 
the IPv6 addresses in pg_hba.conf, but there is no analogous requirement 
for Unix-domain sockets.)  This patch is optional in any case.


0004 fixes a bug in the pg_upgrade test.sh script that was exposed by 
these changes.


0005 fixes up some issues in the test suites.  Right now, the TAP tests 
are hardcoded to not use Unix-domain sockets on Windows, where as 
pg_regress keys off HAVE_UNIX_SOCKETS, which is no longer a useful 
distinguisher.  The change is to not use Unix-domain sockets for all the 
tests by default on Windows (the previous behavior) but give an option 
to use them.  At the moment, I would consider running the test suites 
with Unix-domain sockets enabled as experimental, but that's only 
because of various issues in the test setups.  For instance, there is an 
issue in the comment of pg_regress.c remove_temp() that I'm not sure how 
to address.  Also, the TAP tests don't seem to work because of some path 
issues.  I figured I'd call time on fiddling with this for now and ship 
the patches.


--
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From 9ca09c6f6d59182cdc0c2a28912490471626038a Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Wed, 7 Aug 2019 15:44:19 +0200
Subject: [PATCH v6 1/5] Enable Unix-domain sockets support on Windows

As of Windows 10 version 1803, Unix-domain sockets are supported on
Windows.  But it's not automatically detected by configure because it
looks for struct sockaddr_un and Windows doesn't define that.  So we
just make our own definition on Windows and override the configure
result.

Also set DEFAULT_PGSOCKET_DIR to empty on Windows so by default no
Unix-domain socket is used, because there is no good standard
location.
---
 config/c-library.m4|  5 +++--
 configure  |  5 -
 src/include/c.h|  4 
 src/include/pg_config.h.in |  6 +++---
 src/include/pg_config_manual.h | 15 ---
 src/include/port/win32.h   | 11 +++
 src/tools/msvc/Solution.pm |  2 +-
 7 files changed, 34 insertions(+), 14 deletions(-)

diff --git a/config/c-library.m4 b/config/c-library.m4
index d9a31d7664..163ad5742d 100644
--- a/config/c-library.m4
+++ b/config/c-library.m4
@@ -102,10 +102,11 @@ AC_DEFUN([PGAC_UNION_SEMUN],
 
 # PGAC_STRUCT_SOCKADDR_UN
 # ---
-# If `struct sockaddr_un' exists, define HAVE_UNIX_SOCKETS.
+# If `struct sockaddr_un' exists, define HAVE_STRUCT_SOCKADDR_UN.
+# If it is missing then one could define it.
 # (Requires test for !)
 AC_DEFUN([PGAC_STRUCT_SOCKADDR_UN],
-[AC_CHECK_TYPE([struct sockaddr_un], [AC_DEFINE(HAVE_UNIX_SOCKETS, 1, [Define 
to 1 if you have unix sockets.])], [],
+[AC_CHECK_TYPES([struct sockaddr_un], [], [],
 [#include 
 #ifdef HAVE_SYS_UN_H
 #include 
diff --git a/configure b/configure
index 37aa82dcd4..8b770cb67f 100755
--- a/configure
+++ b/configure
@@ -14061,7 +14061,10 @@ ac_fn_c_check_type "$LINENO" "struct sockaddr_un" 
"ac_cv_type_struct_sockaddr_un
 "
 if test "x$ac_cv_type_struct_sockaddr_un" = xyes; then :
 
-$as_echo "#define HAVE_UNIX_SOCKETS 1" >>confdefs.h
+cat >>confdefs.h <<_ACEOF
+#define HAVE_STRUCT_SOCKADDR_UN 1
+_ACEOF
+
 
 fi
 
diff --git a/src/include/c.h b/src/include/c.h
index d10b9812fb..9562569d2a 100644
--- a/src/include/c.h
+++ b/src/include/c.h
@@ -1077,6 +1077,10 @@ extern void ExceptionalCondition(const char 
*conditionName,
  * 
  */
 
+#ifdef HAVE_STRUCT_SOCKADDR_UN
+#define HAVE_UNIX_SOCKETS 1
+#endif
+
 /*
  * Invert the sign of a qsort-style comparison result, ie, exchange negative
  * and positive integer values, being careful not to get the wrong answer
diff --git a/src/include/pg_config.h.in b/src/include/pg_config.h.in
index 60dcf42974..8449e02123 100644
--- a/src/include/pg_config.h.in
+++ b/src/include/pg_config.h.in
@@ -605,6 +605,9 

Re: Add %x to PROMPT1 and PROMPT2

[Vik Fearing]

On 12/02/2020 05:35, Michael Paquier wrote:
> On Tue, Feb 11, 2020 at 10:05:25AM -0500, Robert Haas wrote:
>> No objections here. I'm glad that we put in the effort to get more
>> opinions, but I agree that an overall vote of ~58 to ~8 is a pretty
>> strong consensus.
> 
> Clearly, so done as dcdbb5a.

Thanks!
-- 
Vik Fearing