Re: [HACKERS] Missing checks when malloc returns NULL...

[Michael Paquier]

On Mon, Aug 29, 2016 at 11:13 PM, Aleksander Alekseev
 wrote:
>> Hello, Michael
>>
>> > I don't know how you did it, but this email has visibly broken the
>> > original thread. Did you change the topic name?
>>
>> I'm very sorry for this. I had no local copy of this thread. So I wrote a
>> new email with the same subject hoping it will be OK. Apparently right
>> In-Reply-To header is also required.
>>
>> >   if (prodesc->user_proname == NULL || prodesc->internal_proname == NULL)
>> > + {
>> > +free(prodesc);
>>
>> I think that prodesc->user_proname and prodesc->internal_proname should
>> also be freed if they are not NULL's.
>>
>> > By the way maybe someone knows other procedures besides malloc, realloc
>> > and strdup that require special attention?
>>
>> I recalled that there is also calloc(). I've found four places that use
>> calloc() and look suspicious to me (see attachment). What do you think -
>> are these bugs or not?

./src/backend/storage/buffer/localbuf.c:LocalBufferBlockPointers =
(Block *) calloc(nbufs, sizeof(Block));
./src/interfaces/libpq/fe-print.c-  fprintf(stderr,
libpq_gettext("out of memory\n"));
Here it does not matter the process is taken down with FATAL or
abort() immediately.

./src/backend/bootstrap/bootstrap.c:app = Typ =
ALLOC(struct typmap *, i + 1);
But here it does actually matter.

> I've just realized that there is also malloc-compatible ShmemAlloc().
> Apparently it's return value sometimes is not properly checked too. See
> attachment.

./src/backend/storage/lmgr/proc.c:  pgxacts = (PGXACT *)
ShmemAlloc(TotalProcs * sizeof(PGXACT));
./src/backend/storage/lmgr/proc.c:  ProcStructLock = (slock_t *)
ShmemAlloc(sizeof(slock_t));
./src/backend/storage/lmgr/lwlock.c:ptr = (char *)
ShmemAlloc(spaceLocks);
./src/backend/storage/ipc/shmem.c:  ShmemAlloc(sizeof(*ShmemVariableCache));
./src/backend/access/transam/slru.c:shared->buffer_locks =
(LWLockPadded *) ShmemAlloc(sizeof(LWLockPadded) * nslots);
./src/backend/postmaster/postmaster.c:  ShmemBackendArray = (Backend
*) ShmemAlloc(size);

The funny part here is that ProcGlobal->allProcs is actually handled,
but not the two others. Well yes, you are right, we really need to
fail on FATAL for all of them if ShmemAlloc returns NULL as they
involve the shmem initialization at postmaster startup.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pg_dump with tables created in schemas created by extensions

[Michael Paquier]

On Tue, Aug 30, 2016 at 5:43 AM, Martín Marqués  wrote:
> This is v4 of the patch, which is actually a cleaner version from the
> v2 one Michael sent.
>
> I stripped off the external index created from the tests as that index
> shouldn't be dumped (table it belongs to isn't dumped, so neither
> should the index). I also took off a test which was duplicated.
>
> I think this patch is a very good first approach. Future improvements
> can be made for indexes, but we need to get the extension dependencies
> right first. That could be done later, on a different patch.
>
> Thoughts?

Let's do as you suggest then, and just focus on the schema issue. I
just had an extra look at the patch and it looks fine to me. So the
patch is now switched as ready for committer.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Re: [sqlsmith] FailedAssertion("!(XLogCtl->Insert.exclusiveBackup)", File: "xlog.c", Line: 10200)

[Michael Paquier]

On Tue, Aug 30, 2016 at 1:44 PM, Fujii Masao  wrote:
> On Tue, Aug 30, 2016 at 1:32 PM, Michael Paquier
>  wrote:
>> On Mon, Aug 29, 2016 at 11:16 PM, Fujii Masao  wrote:
>>> You seem to add another entry for this patch into CommitFest.
>>> Either of them needs to be removed.
>>> https://commitfest.postgresql.org/10/698/
>>
>> Indeed. I just removed this one.
>>
>>> This patch prevents pg_stop_backup from starting while pg_start_backup
>>> is running. OTOH, we also should prevent pg_start_backup from starting
>>> until "previous" pg_stop_backup has completed? What happens if
>>> pg_start_backup starts while pg_stop_backup is running?
>>> As far as I read the current code, ISTM that there is no need to do that,
>>> but it's better to do the double check.
>>
>> I don't agree about doing that.
>
> Hmm... my previous comment was confusing. I wanted to comment "it's better
> *also for you* to do the double check whether we need to prevent 
> pg_start_backup
> while pg_stop_backup is running or not". Your answer seems to be almost the 
> same
> as mine, i.e., not necessary. Right?

Yes, that's not necessary to do more. In the worst case, say
pg_start_backup starts when pg_stop_backup is running. And
pg_stop_backup has decremented the backup counters, but not yet
removed the backup_label, then pg_start_backup would just choke on the
presence of the backup_label file.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] Send numeric version to clients

[Craig Ringer]

On 30 August 2016 at 00:37, Robert Haas  wrote:

> Long story short, I kind of agree that it might have been better to
> expose server_version_num rather than server_version in the beginning,
> but I'm not sure that it really helps anybody now, especially given
> our decision to simplify the version number format going forward.

OK, that's that then. We can fix this properly when the fabled v4
protocol moves into the real world, and keep hacking around it in the
mean time. No point restating my disagreement for the 1000th time,
done is done and better things for us all to spend our time on.

-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Re: [sqlsmith] FailedAssertion("!(XLogCtl->Insert.exclusiveBackup)", File: "xlog.c", Line: 10200)

[Fujii Masao]

On Tue, Aug 30, 2016 at 1:32 PM, Michael Paquier
 wrote:
> On Mon, Aug 29, 2016 at 11:16 PM, Fujii Masao  wrote:
>> You seem to add another entry for this patch into CommitFest.
>> Either of them needs to be removed.
>> https://commitfest.postgresql.org/10/698/
>
> Indeed. I just removed this one.
>
>> This patch prevents pg_stop_backup from starting while pg_start_backup
>> is running. OTOH, we also should prevent pg_start_backup from starting
>> until "previous" pg_stop_backup has completed? What happens if
>> pg_start_backup starts while pg_stop_backup is running?
>> As far as I read the current code, ISTM that there is no need to do that,
>> but it's better to do the double check.
>
> I don't agree about doing that.

Hmm... my previous comment was confusing. I wanted to comment "it's better
*also for you* to do the double check whether we need to prevent pg_start_backup
while pg_stop_backup is running or not". Your answer seems to be almost the same
as mine, i.e., not necessary. Right?

Regards,

-- 
Fujii Masao


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Re: [sqlsmith] FailedAssertion("!(XLogCtl->Insert.exclusiveBackup)", File: "xlog.c", Line: 10200)

[Michael Paquier]

On Mon, Aug 29, 2016 at 11:16 PM, Fujii Masao  wrote:
> You seem to add another entry for this patch into CommitFest.
> Either of them needs to be removed.
> https://commitfest.postgresql.org/10/698/

Indeed. I just removed this one.

> This patch prevents pg_stop_backup from starting while pg_start_backup
> is running. OTOH, we also should prevent pg_start_backup from starting
> until "previous" pg_stop_backup has completed? What happens if
> pg_start_backup starts while pg_stop_backup is running?
> As far as I read the current code, ISTM that there is no need to do that,
> but it's better to do the double check.

I don't agree about doing that. The on-disk presence of the
backup_label file is what prevents pg_start_backup to run should
pg_stop_backup have already decremented its counters but not unlinked
yet the backup_label, so this insurance is really enough IMO. One good
reason to keep pg_stop_backup as-is in its failure handling is that if
for example it fails to remove the backup_label file, it is still
possible to do again a backup by just removing manually the existing
backup_label file *without* restarting the server. If you have an
in-memory state it would not be possible to fallback to this method
and you'd need a method to clean up the in-memory state.

Now, well, with the patch as well as HEAD, it could be possible that
the backup counters are decremented, but pg_stop_backup *fails* to
remove the backup_label file which would prevent any subsequent
pg_start_backup to run, because there is still a backup_label file, as
well as any other pg_stop_backup to run, because there is no backup
running per the in-memory state. We could improve the in-memory state
by:
- Having an extra exclusive backup status to mark an exclusive backup
as stopping, say EXCLUSIVE_BACKUP_STOPPING. Then mark the exclusive
backup status as such when do_pg_stop_backup starts.
- delaying counter decrementation in pg_stop_backup to the moment when
the backup_label file has been removed.
- Taking an extra time the exclusive WAL insert lock after
backup_label is removed, and decrement the counters.
- During the time the backup_label file is removed, we need an error
callback to switch back to BACKUP_RUNNING in case of error, similarly
to do_pg_start_backup.
Which is more or less the attached patch. Now, if pg_stop_backup
fails, this has the disadvantage to force a server restart to clean up
the in-memory state, instead of just removing manually the existing
backup_file.

For those reasons I still strongly recommend using v3, and not meddle
with the error handling of pg_stop_backup. Because, well, that's
actually not necessary, and this would just hurt the error handling of
exclusive backups.
-- 
Michael
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index acd95aa..ad04e3e 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -460,6 +460,28 @@ typedef union WALInsertLockPadded
 } WALInsertLockPadded;
 
 /*
+ * State of an exclusive backup
+ *
+ * EXCLUSIVE_BACKUP_NONE means that there is no exclusive backup actually
+ * running, to be more precise pg_start_backup() is not being executed for
+ * an exclusive backup and there is exclusive backup in progress.
+ * EXCLUSIVE_BACKUP_STARTING means that pg_start_backup() is running and
+ * that is is not done yet.
+ * EXCLUSIVE_BACKUP_STOPPING means that pg_stop_backup() is running and
+ * that is is not done yet.
+ * EXCLUSIVE_BACKUP_IN_PROGRESS means that pg_start_backup() has finished
+ * running and an exclusive backup is in progress. pg_stop_backup() is
+ * needed to finish it.
+ */
+typedef enum ExclusiveBackupState
+{
+	EXCLUSIVE_BACKUP_NONE = 0,
+	EXCLUSIVE_BACKUP_STARTING,
+	EXCLUSIVE_BACKUP_STOPPING,
+	EXCLUSIVE_BACKUP_IN_PROGRESS
+} ExclusiveBackupState;
+
+/*
  * Shared state data for WAL insertion.
  */
 typedef struct XLogCtlInsert
@@ -500,13 +522,14 @@ typedef struct XLogCtlInsert
 	bool		fullPageWrites;
 
 	/*
-	 * exclusiveBackup is true if a backup started with pg_start_backup() is
-	 * in progress, and nonExclusiveBackups is a counter indicating the number
+	 * exclusiveBackupState is changed to EXCLUSIVE_BACKUP_STARTING for the
+	 * duration of pg_start_backup(), then to EXCLUSIVE_BACKUP_IN_PROGRESS once
+	 * it is done, and nonExclusiveBackups is a counter indicating the number
 	 * of streaming base backups currently in progress. forcePageWrites is set
 	 * to true when either of these is non-zero. lastBackupStart is the latest
 	 * checkpoint redo location used as a starting point for an online backup.
 	 */
-	bool		exclusiveBackup;
+	ExclusiveBackupState exclusiveBackupState;
 	int			nonExclusiveBackups;
 	XLogRecPtr	lastBackupStart;
 
@@ -842,6 +865,7 @@ static void xlog_outrec(StringInfo buf, XLogReaderState *record);
 #endif
 static void xlog_outdesc(StringInfo buf, XLogReaderState *record);
 static void pg_start_backup_callback(int code, Datum arg);
+static void 

[HACKERS] Comment on GatherPath.single_copy

[Kyotaro HORIGUCHI]

Hello.

The comment on GatherPath.single_copy is the following.

===
/*
 * GatherPath runs several copies of a plan in parallel and collects the
 * results.  The parallel leader may also execute the plan, unless the
 * single_copy flag is set.
 */
typedef struct GatherPath
{
Pathpath;
Path   *subpath;/* path for each worker */
boolsingle_copy;/* path must not be executed >1x */
} GatherPath;
===

The ">1x" looks to me as a kind of typo but looking the comment
above the struct it came to look as "more than once (or one
copy)". But it seems to me that it would be better to be in
ordinary words.


> boolsingle_copy;/* path must not be executed multiply */

If anyone feel that it is confusing with a verb form, the
following might be better.

> bool  single_copy;/* path must not span on multiple processes */

Since anyway I cannot find a comfortable expression for this, I
attached a patch that does the last one.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/backend/executor/nodeGather.c b/src/backend/executor/nodeGather.c
index e4cfc44..ed9c71f 100644
--- a/src/backend/executor/nodeGather.c
+++ b/src/backend/executor/nodeGather.c
@@ -312,6 +312,7 @@ gather_getnext(GatherState *gatherstate)
 
 		if (gatherstate->need_to_scan_locally)
 		{
+			elog(LOG, "EXECLOCAL");
 			outerTupleSlot = ExecProcNode(outerPlan);
 
 			if (!TupIsNull(outerTupleSlot))
@@ -385,15 +386,18 @@ gather_readnext(GatherState *gatherstate)
 
 		/* Have we visited every (surviving) TupleQueueReader? */
 		nvisited++;
+		elog(LOG, "NEXT");
 		if (nvisited >= gatherstate->nreaders)
 		{
 			/*
 			 * If (still) running plan locally, return NULL so caller can
 			 * generate another tuple from the local copy of the plan.
 			 */
+			elog(LOG, "WAIT0");
 			if (gatherstate->need_to_scan_locally)
 return NULL;
 
+			elog(LOG, "WAIT");
 			/* Nothing to do except wait for developments. */
 			WaitLatch(MyLatch, WL_LATCH_SET, 0);
 			ResetLatch(MyLatch);
diff --git a/src/include/nodes/relation.h b/src/include/nodes/relation.h
index fcfb0d4..b6b9779 100644
--- a/src/include/nodes/relation.h
+++ b/src/include/nodes/relation.h
@@ -1189,7 +1189,7 @@ typedef struct GatherPath
 {
 	Path		path;
 	Path	   *subpath;		/* path for each worker */
-	bool		single_copy;	/* path must not be executed >1x */
+	bool		single_copy;	/* path must not span on multiple processes */
 } GatherPath;
 
 /*

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] Send numeric version to clients

[Craig Ringer]

On 30 Aug 2016 9:07 AM, "Dave Cramer"  wrote:
>
>
> On 29 August 2016 at 15:42, Tom Lane  wrote:
>>
>> Kevin Grittner  writes:
>> > Regarding Java, for anything above the driver itself the
>> > JDBC API says the DatabaseMetaData class must implement these
>> > methods:
>> > ...
>> > That *should* make it just a problem for the driver itself.  That
>> > seems simple enough until you check what those methods have been
>> > returning so far.
>>
>> Seems like we just make getDatabaseMinorVersion() return zero for
>> any major >= 10.  That might not have been the best thing to do in
>> a green field, but given the precedent ...
>>
>> regards, tom lane
>>
>>
> seems to me that it should report 10 for the major and whatever comes
after the . for the minor ?

IMO it just needs to be consistent with the numeric version we report
elsewhere - PG_VERSION_NUM, server_version_num etc.

>
> Dave Cramer
>
> da...@postgresintl.com
> www.postgresintl.com
>

Re: [HACKERS] asynchronous and vectorized execution

[Kyotaro HORIGUCHI]

No, it was wrong.

At Mon, 29 Aug 2016 17:08:36 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI 
 wrote in 
<20160829.170836.161449399.horiguchi.kyot...@lab.ntt.co.jp>
> Hello,
> 
> I considered applying the async infrastructure onto nodeGather,
> but since parallel workers hardly make Gather (or the leader)
> wait, it's really useless at least for simple cases. Furthermore,
> as several people may have said before, being defferent from
> foreign scans, gather (or other kinds of parallel) nodes usually
> have several workers and will have up to two digit nubmers at the
> most even on so-called many-core boxes. I finally gave up
> applying this to nodeGather.

I overlooked that local scan takes place instead of waiting
workers to be ready. I will reconsider counting that..

> As the result, the attached patchset is functionally the same
> with the last version but replace misused Assert with
> AssertMacro.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center




-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Re: [COMMITTERS] pgsql: Change the way that LWLocks for extensions are allocated.

[Andres Freund]

On 2016-08-30 07:57:19 +0530, Amit Kapila wrote:
> I will write such a test case either in this week or early next week.

Great.

> I hope this is not super urgent, let me know if you think otherwise.

It's not urgent, no.

Thanks!

Andres


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Re: [COMMITTERS] pgsql: Change the way that LWLocks for extensions are allocated.

[Amit Kapila]

On Mon, Aug 29, 2016 at 10:09 PM, Robert Haas  wrote:
> On Sat, Aug 27, 2016 at 3:30 AM, Andres Freund  wrote:
>> Hi,
>>
>> On 2016-02-04 21:43:14 +, Robert Haas wrote:
>>> Change the way that LWLocks for extensions are allocated.
>>>
>>> The previous RequestAddinLWLocks() method had several disadvantages.
>>> First, the locks would be in the main tranche; we've recently decided
>>> that it's useful for LWLocks used for separate purposes to have
>>> separate tranche IDs.  Second, there wasn't any correlation between
>>> what code called RequestAddinLWLocks() and what code called
>>> LWLockAssign(); when multiple modules are in use, it could become
>>> quite difficult to troubleshoot problems where LWLockAssign() ran out
>>> of locks.  To fix, create a concept of named LWLock tranches which
>>> can be used either by extension or by core code.
>>>
>>> Amit Kapila and Robert Haas
>>
>> I noticed that this code has no test coverage:
>>
>> http://coverage.postgresql.org/src/backend/storage/lmgr/lwlock.c.gcov.html
>>
>> It'd be good to add some, although I'm not entirely sure what the best
>> way is. Maybe add a simple pg_stat_statements test?
>
> That would also have the advantage of improving the test coverage for
> pg_stat_statements, which is at the moment quite a bit thinner than
> the coverage for lwlock.c.
>

I will write such a test case either in this week or early next week.
I hope this is not super urgent, let me know if you think otherwise.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Write Ahead Logging for Hash Indexes

[Amit Kapila]

On Tue, Aug 30, 2016 at 3:40 AM, Alvaro Herrera
 wrote:
> Amit Kapila wrote:
>
>> How about attached?
>
> That works; pushed.

Thanks.

> (I removed a few #includes from the new file.)
>

oops, copied from hash.h and forgot to remove those.

>> If you want, I think we can one step further and move hash_redo to a
>> new file hash_xlog.c which is required for main patch, but we can
>> leave it for later as well.
>
> I think that can be a part of the main patch.
>

Okay, makes sense.  Any suggestions on splitting the main patch or in
general on design/implementation is welcome.

I will rebase the patches on the latest commit.  Current status is
that, I have fixed the problems reported by Jeff Janes and Mark
Kirkwood.  Currently, we are doing the stress testing of the patch
using pgbench, once that is done, will post a new version.  I am
hoping that it will be complete in this week.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Renaming of pg_xlog and pg_clog

[Michael Paquier]

On Mon, Aug 29, 2016 at 9:39 PM, Craig Ringer
 wrote:
> Cool. I'll mark as waiting on author pending that.
>
> It'll be good to get this footgun put away.

OK, so done. I have put the renaming of pg_xlog to pg_wal on top patch
stack as that's the one making no discussion it seems: a lot of people
like pg_wal. I have added as well handling for the renaming in
pg_basebackup by using PQserverVersion. One thing to note is that a
connection needs to be made to the target server *before* creating the
soft link of pg_xlog/pg_wal because we need to know the version of the
target server. pg_upgrade is handled as well. And that's all in 0001.

Patch 0002 does pg_clog -> pg_trans, "trans" standing for
"transaction". Switching to pg_trans_status or pg_xact_status is not
that complicated to change anyway...
-- 
Michael
From 9806c07cbb5bd31946b48274dde8b5db65aa6169 Mon Sep 17 00:00:00 2001
From: Michael Paquier 
Date: Mon, 29 Aug 2016 15:05:46 +0900
Subject: [PATCH 2/2] Rename pg_clog to pg_trans

pg_upgrade is updated to handle the transfer correctly to post-10
clusters.
---
 doc/src/sgml/backup.sgml   |  4 ++--
 doc/src/sgml/catalogs.sgml |  4 ++--
 doc/src/sgml/config.sgml   |  2 +-
 doc/src/sgml/func.sgml |  2 +-
 doc/src/sgml/maintenance.sgml  |  8 
 doc/src/sgml/ref/pg_resetxlog.sgml |  4 ++--
 doc/src/sgml/ref/pg_rewind.sgml|  2 +-
 doc/src/sgml/storage.sgml  | 10 +-
 doc/src/sgml/wal.sgml  |  2 +-
 src/backend/access/heap/heapam.c   |  4 ++--
 src/backend/access/transam/README  | 12 ++--
 src/backend/access/transam/clog.c  |  2 +-
 src/backend/access/transam/commit_ts.c |  2 +-
 src/backend/access/transam/multixact.c |  2 +-
 src/backend/access/transam/subtrans.c  |  4 ++--
 src/backend/access/transam/transam.c   |  2 +-
 src/backend/access/transam/twophase.c  |  4 ++--
 src/backend/access/transam/xact.c  | 18 +-
 src/backend/access/transam/xlog.c  |  2 +-
 src/backend/commands/vacuum.c  | 10 +-
 src/backend/postmaster/autovacuum.c|  2 +-
 src/backend/storage/buffer/README  |  2 +-
 src/backend/storage/ipc/procarray.c|  4 ++--
 src/backend/utils/time/tqual.c |  6 +++---
 src/bin/initdb/initdb.c|  2 +-
 src/bin/pg_upgrade/exec.c  |  6 ++
 src/bin/pg_upgrade/pg_upgrade.c| 30 ++
 src/include/access/slru.h  |  4 ++--
 28 files changed, 84 insertions(+), 72 deletions(-)

diff --git a/doc/src/sgml/backup.sgml b/doc/src/sgml/backup.sgml
index 074a6b0..1fa4d0e 100644
--- a/doc/src/sgml/backup.sgml
+++ b/doc/src/sgml/backup.sgml
@@ -382,10 +382,10 @@ tar -cf backup.tar /usr/local/pgsql/data
   directories. This will not work because the
   information contained in these files is not usable without
   the commit log files,
-  pg_clog/*, which contain the commit status of
+  pg_trans/*, which contain the commit status of
   all transactions. A table file is only usable with this
   information. Of course it is also impossible to restore only a
-  table and the associated pg_clog data
+  table and the associated pg_trans data
   because that would render all other tables in the database
   cluster useless.  So file system backups only work for complete
   backup and restoration of an entire database cluster.
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 4e09e06..783d49c 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -1847,7 +1847,7 @@
All transaction IDs before this one have been replaced with a permanent
(frozen) transaction ID in this table.  This is used to track
whether the table needs to be vacuumed in order to prevent transaction
-   ID wraparound or to allow pg_clog to be shrunk.  Zero
+   ID wraparound or to allow pg_trans to be shrunk.  Zero
(InvalidTransactionId) if the relation is not a table.
   
  
@@ -2514,7 +2514,7 @@
All transaction IDs before this one have been replaced with a permanent
(frozen) transaction ID in this database.  This is used to
track whether the database needs to be vacuumed in order to prevent
-   transaction ID wraparound or to allow pg_clog to be shrunk.
+   transaction ID wraparound or to allow pg_trans to be shrunk.
It is the minimum of the per-table
pg_class.relfrozenxid values.
   
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 8e0bccc..dff2945 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -5816,7 +5816,7 @@ COPY postgres_log FROM '/full/path/to/logfile.csv' WITH csv;
 

 Vacuum also allows removal of old files from the
-pg_clog subdirectory, which is why the default
+pg_trans 

Re: [HACKERS] Odd oid-system-column handling in postgres_fdw

[Etsuro Fujita]

On 2016/08/26 22:35, Heikki Linnakangas wrote:

On 04/05/2016 11:15 AM, Etsuro Fujita wrote:

On 2016/03/16 16:25, Etsuro Fujita wrote:

PG9.5 allows us to add an oid system column to foreign tables, using
ALTER FOREIGN TABLE SET WITH OIDS, but currently, that column reads as
zeroes in postgres_fdw.  That seems to me like a bug.  So, I'd like to
propose to fix that, by retrieving that column from the remote server
when requested.  I'm attaching a proposed patch for that.



I rebased the patch against HEAD.  Updated patch attached.



Committed, thanks!


Thank you for taking the time to commit this patch!

Best regards,
Etsuro Fujita




--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] Send numeric version to clients

[Dave Cramer]

On 29 August 2016 at 15:42, Tom Lane  wrote:

> Kevin Grittner  writes:
> > Regarding Java, for anything above the driver itself the
> > JDBC API says the DatabaseMetaData class must implement these
> > methods:
> > ...
> > That *should* make it just a problem for the driver itself.  That
> > seems simple enough until you check what those methods have been
> > returning so far.
>
> Seems like we just make getDatabaseMinorVersion() return zero for
> any major >= 10.  That might not have been the best thing to do in
> a green field, but given the precedent ...
>
> regards, tom lane
>
>
> seems to me that it should report 10 for the major and whatever comes
after the . for the minor ?


Dave Cramer

da...@postgresintl.com
www.postgresintl.com

Re: [HACKERS] Renaming of pg_xlog and pg_clog

[Peter Eisentraut]

On 8/28/16 8:45 PM, Jim Nasby wrote:
> People accidentally blowing away pg_clog or pg_xlog is a pretty common 
> occurrence, and I don't think there's all that many tools that reference 
> them. I think it's well worth renaming them.

I think a related problem is that the default log directory is called
"pg_log", which is very similar to those other two.  There is no quick
way to tell their meaning apart.

I would consider changing the default from "pg_log" to "log".  Then we'd
also be at the point where we can say, everything starting with "pg_" is
internal, don't touch it.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Renaming of pg_xlog and pg_clog

[Peter Eisentraut]

On 8/29/16 12:54 PM, Robert Haas wrote:
> As for the new names, how about pg_wal and
> pg_xact?  I don't think "pg_trans" is as good; is it transactional or
> transient or transport or transitive or what?

pg_transaction_status? (or pg_xact_status if you want to keep it short)

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Renaming of pg_xlog and pg_clog

[Andres Freund]

On 2016-08-29 19:27:29 -0500, Jim Nasby wrote:
> On 8/27/16 1:11 PM, Tom Lane wrote:
> > Alvaro Herrera  writes:
> > > I'm for renaming too, but I'd go with Peter E's suggestion: move pg_xlog
> > > to something like $PGDATA/var/wal or $PGDATA/srv/wal or something like 
> > > that.
> > 
> > I think that would make sense if we were to relocate *everything* under
> > PGDATA into some FHS-like subdirectory structure.  But I'm against moving
> > the config files for previously-stated reasons, and I doubt it makes sense
> > to adopt an FHS-like structure only in part.
> 
> What if we left symlinks for the config files? Or perhaps even better,
> provide a tool that will create them for people that actually need
> them.

See the thread around
http://archives.postgresql.org/message-id/20160826104446.n3cif4m7modslkrs%40msg.df7cb.de


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Renaming of pg_xlog and pg_clog

[Jim Nasby]

On 8/27/16 1:11 PM, Tom Lane wrote:

Alvaro Herrera  writes:

I'm for renaming too, but I'd go with Peter E's suggestion: move pg_xlog
to something like $PGDATA/var/wal or $PGDATA/srv/wal or something like that.


I think that would make sense if we were to relocate *everything* under
PGDATA into some FHS-like subdirectory structure.  But I'm against moving
the config files for previously-stated reasons, and I doubt it makes sense
to adopt an FHS-like structure only in part.


What if we left symlinks for the config files? Or perhaps even better, 
provide a tool that will create them for people that actually need them.

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)   mobile: 512-569-9461


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] OpenSSL 1.1 breaks configure and more

[Andreas Karlsson]

On 08/26/2016 11:31 AM, Heikki Linnakangas wrote:

On 07/05/2016 04:46 PM, Andreas Karlsson wrote:

@@ -280,8 +287,9 @@ px_find_digest(const char *name, PX_MD **res)
 digest = px_alloc(sizeof(*digest));
 digest->algo = md;

-EVP_MD_CTX_init(>ctx);
-if (EVP_DigestInit_ex(>ctx, digest->algo, NULL) == 0)
+digest->ctx = EVP_MD_CTX_create();
+EVP_MD_CTX_init(digest->ctx);
+if (EVP_DigestInit_ex(digest->ctx, digest->algo, NULL) == 0)
 return -1;

 h = px_alloc(sizeof(*h));


Now that we're calling EVP_MD_CTX_create((), which allocates memory, are
we risking memory leaks? It has always been part of the contract that
you have to call px_md_free(), for any context returned by
px_find_digest(), but I wonder just how careful we have been about that.
Before this, you would probably get away with it without leaking, if the
digest implementation didn't allocate any extra memory or other resources.

At least pg_digest and try_unix_std functions call px_find_digest(), and
then do more palloc()s which could elog() if you run out of memory,
leaking th digest struct. Highly unlikely, but I think it would be
fairly straightforward to reorder those calls to eliminate the risk, so
we probably should.


Since px_find_digest() calls palloc() later in the function there is a 
slim possibility of memory leaks. How do we generally handle that things 
not allocated with palloc() may leak when something calls elog()?


I have attached new versions of the patches which are rebased on master, 
with slightly improves error handling in px_find_digest(), and handles 
the deprecation of ASN1_STRING_data().


Andreas
>From dea78efc9a4b68f2704dcf8cb089c0b45f3f385b Mon Sep 17 00:00:00 2001
From: Andreas Karlsson 
Date: Tue, 28 Jun 2016 05:55:03 +0200
Subject: [PATCH 1/3] Fixes for compiling with OpenSSL 1.1

- Check for SSL_new now that SSL_library_init is a macro
- Do not access struct members directly
- RAND_SSLeay was renamed to RAND_OpenSSL

squash! Fixes for compiling with OpenSSL 1.1
---
 configure| 44 
 configure.in |  4 +--
 contrib/pgcrypto/openssl.c   | 30 ++
 contrib/sslinfo/sslinfo.c| 14 ++
 src/backend/libpq/be-secure-openssl.c| 39 +++-
 src/interfaces/libpq/fe-secure-openssl.c | 39 +++-
 6 files changed, 100 insertions(+), 70 deletions(-)

diff --git a/configure b/configure
index 45c8eef..caf6f26 100755
--- a/configure
+++ b/configure
@@ -9538,9 +9538,9 @@ else
   as_fn_error $? "library 'crypto' is required for OpenSSL" "$LINENO" 5
 fi
 
- { $as_echo "$as_me:${as_lineno-$LINENO}: checking for SSL_library_init in -lssl" >&5
-$as_echo_n "checking for SSL_library_init in -lssl... " >&6; }
-if ${ac_cv_lib_ssl_SSL_library_init+:} false; then :
+ { $as_echo "$as_me:${as_lineno-$LINENO}: checking for SSL_new in -lssl" >&5
+$as_echo_n "checking for SSL_new in -lssl... " >&6; }
+if ${ac_cv_lib_ssl_SSL_new+:} false; then :
   $as_echo_n "(cached) " >&6
 else
   ac_check_lib_save_LIBS=$LIBS
@@ -9554,27 +9554,27 @@ cat confdefs.h - <<_ACEOF >conftest.$ac_ext
 #ifdef __cplusplus
 extern "C"
 #endif
-char SSL_library_init ();
+char SSL_new ();
 int
 main ()
 {
-return SSL_library_init ();
+return SSL_new ();
   ;
   return 0;
 }
 _ACEOF
 if ac_fn_c_try_link "$LINENO"; then :
-  ac_cv_lib_ssl_SSL_library_init=yes
+  ac_cv_lib_ssl_SSL_new=yes
 else
-  ac_cv_lib_ssl_SSL_library_init=no
+  ac_cv_lib_ssl_SSL_new=no
 fi
 rm -f core conftest.err conftest.$ac_objext \
 conftest$ac_exeext conftest.$ac_ext
 LIBS=$ac_check_lib_save_LIBS
 fi
-{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $ac_cv_lib_ssl_SSL_library_init" >&5
-$as_echo "$ac_cv_lib_ssl_SSL_library_init" >&6; }
-if test "x$ac_cv_lib_ssl_SSL_library_init" = xyes; then :
+{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $ac_cv_lib_ssl_SSL_new" >&5
+$as_echo "$ac_cv_lib_ssl_SSL_new" >&6; }
+if test "x$ac_cv_lib_ssl_SSL_new" = xyes; then :
   cat >>confdefs.h <<_ACEOF
 #define HAVE_LIBSSL 1
 _ACEOF
@@ -9644,9 +9644,9 @@ else
   as_fn_error $? "library 'eay32' or 'crypto' is required for OpenSSL" "$LINENO" 5
 fi
 
- { $as_echo "$as_me:${as_lineno-$LINENO}: checking for library containing SSL_library_init" >&5
-$as_echo_n "checking for library containing SSL_library_init... " >&6; }
-if ${ac_cv_search_SSL_library_init+:} false; then :
+ { $as_echo "$as_me:${as_lineno-$LINENO}: checking for library containing SSL_new" >&5
+$as_echo_n "checking for library containing SSL_new... " >&6; }
+if ${ac_cv_search_SSL_new+:} false; then :
   $as_echo_n "(cached) " >&6
 else
   ac_func_search_save_LIBS=$LIBS
@@ -9659,11 +9659,11 @@ cat confdefs.h - <<_ACEOF >conftest.$ac_ext
 #ifdef __cplusplus
 extern "C"
 #endif
-char SSL_library_init ();
+char SSL_new ();
 int
 main ()
 {
-return SSL_library_init ();
+return SSL_new ();
   ;
   

Re: [HACKERS] Re: [COMMITTERS] pgsql: Change the way that LWLocks for extensions are allocated.

[Alvaro Herrera]

Robert Haas wrote:

> That would also have the advantage of improving the test coverage for
> pg_stat_statements, which is at the moment quite a bit thinner than
> the coverage for lwlock.c.

Hmm, the coverage-html report is not currently covering contrib ... I
suppose that's an easily fixable oversight.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Write Ahead Logging for Hash Indexes

[Alvaro Herrera]

Amit Kapila wrote:

> How about attached?

That works; pushed.  (I removed a few #includes from the new file.)

> If you want, I think we can one step further and move hash_redo to a
> new file hash_xlog.c which is required for main patch, but we can
> leave it for later as well.

I think that can be a part of the main patch.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] OpenSSL 1.1 breaks configure and more

[Andreas Karlsson]

On 08/29/2016 07:22 PM, Heikki Linnakangas wrote:

Pushed with some small doc fixes, thanks Andreas! I'll continue
reviewing the rest of the patches.


Thanks!

Andreas



--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pg_dump with tables created in schemas created by extensions

[Martín Marqués]

Hi,

This is v4 of the patch, which is actually a cleaner version from the
v2 one Michael sent.

I stripped off the external index created from the tests as that index
shouldn't be dumped (table it belongs to isn't dumped, so neither
should the index). I also took off a test which was duplicated.

I think this patch is a very good first approach. Future improvements
can be made for indexes, but we need to get the extension dependencies
right first. That could be done later, on a different patch.

Thoughts?

-- 
Martín Marquéshttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services


pgdump-extension-v4.patch
Description: invalid/octet-stream

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Fix some corner cases that cube_in rejects

[Tom Lane]

Greg Stark  writes:
> On Mon, Aug 29, 2016 at 7:19 PM, Tom Lane  wrote:
>> To deal with the infinity/NaN issues, I made cube_in and cube_out rely
>> on float8in_internal and float8out_internal, as we recently did for the
>> core geometric types.  That causes the response to "1e-700" to be an
>> out-of-range error rather than silent underflow, which seems to me to
>> be fine, especially since it removes the platform dependency that's
>> responsible for needing the separate cube_1.out and cube_3.out files.

> So what happens to a database that has invalid cubes in it already?
> Offhand I suspect they mostly become valid since float8in will handle
> NaN and the like.

Nothing really.  cube_out works fine.  The point of substituting
float8out_internal is mostly to make sure we get platform-independent
spellings for inf and nan.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Fix some corner cases that cube_in rejects

[Greg Stark]

On Mon, Aug 29, 2016 at 7:19 PM, Tom Lane  wrote:
> To deal with the infinity/NaN issues, I made cube_in and cube_out rely
> on float8in_internal and float8out_internal, as we recently did for the
> core geometric types.  That causes the response to "1e-700" to be an
> out-of-range error rather than silent underflow, which seems to me to
> be fine, especially since it removes the platform dependency that's
> responsible for needing the separate cube_1.out and cube_3.out files.

So what happens to a database that has invalid cubes in it already?
Offhand I suspect they mostly become valid since float8in will handle
NaN and the like.

Incidentally, I so wish this module were named "vector" instead of
cube. I don't think I was confused by it for ages and still find it
confuses me for a few moments before I remember what it does.

-- 
greg


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pg_dump with tables created in schemas created by extensions

[Martín Marqués]

2016-08-29 4:51 GMT-03:00 Michael Paquier :
>
>> I see the current behavior is documented, and I do understand why global
>> objects can't be part of the extension, but for indexes it seems to violate
>> POLA a bit.
>>
>> Is there a reason why we don't want the extension/index dependencies?
>
> I think that we could do a better effort for indexes at least, in the
> same way as we do for sequences as both are referenced in pg_class. I
> don't know the effort to get that done for < 9.6, but if we can do it
> at least for 9.6 and 10, which is where pg_dump is a bit smarter in
> the way it deals with dependencies, we should do it.

ATM I don't have a strong opinion one way or the other regarding the
dependency of indexes and extensions. I believe we have to put more
thought into it, and at the end we might just leave it as it is.

What I do believe is that this requires a separate thread, and if
agreed, a separate patch from this issue.

I'm going to prepare another patch where I'm going to strip the tests
for external indexes which are failing now. They actually fail
correctly as the table they depend on will not be dumped, so it's the
developer/DB designer who has to take care of these things.

If in the near or not so near future we provide a patch to deal with
these missing dependencies, we can easily patch pg_dump so it deals
with this correctly.

Regards,

-- 
Martín Marquéshttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] Send numeric version to clients

[Tom Lane]

Kevin Grittner  writes:
> Regarding Java, for anything above the driver itself the
> JDBC API says the DatabaseMetaData class must implement these
> methods:
> ...
> That *should* make it just a problem for the driver itself.  That
> seems simple enough until you check what those methods have been
> returning so far.

Seems like we just make getDatabaseMinorVersion() return zero for
any major >= 10.  That might not have been the best thing to do in
a green field, but given the precedent ...

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] Send numeric version to clients

[Kevin Grittner]

On Mon, Aug 29, 2016 at 11:37 AM, Robert Haas  wrote:

> Note that these are all one-liners, and I bet the same is true in
> mostly other languages.  Even in notoriously verbose languages like
> Java or Cobol or ADA it can't be very hard.

Regarding Java, for anything above the driver itself the
JDBC API says the DatabaseMetaData class must implement these
methods:

int getDatabaseMajorVersion()
Retrieves the major version number of the underlying database.
int getDatabaseMinorVersion()
Retrieves the minor version number of the underlying database.
String getDatabaseProductVersion()
Retrieves the version number of this database product.

That *should* make it just a problem for the driver itself.  That
seems simple enough until you check what those methods have been
returning so far.  A somewhat minimal program to return these
values:

import java.sql.*;
public final class PrintVersion
{
public static void main(String[] args) throws Exception
{
Class.forName("org.postgresql.Driver");
Connection con =
DriverManager.getConnection("jdbc:postgresql://localhost/test?user=kgrittn");
DatabaseMetaData dbmd = con.getMetaData();
System.out.println(dbmd.getDatabaseMajorVersion() + "  " +
dbmd.getDatabaseMinorVersion());
con.close();
}
}

... outputs this:

9  6

I'm not sure what the right thing to do is here.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] OpenSSL 1.1 breaks configure and more

[Rémi Zara]

> Le 29 août 2016 à 19:46, Heikki Linnakangas  a écrit :
> 
> 
> Tom, Rémi, can you fix locust and prairiedog, please, by updating OpenSSL or 
> removing --with-openssl?
> 

Hi,

Should be OK for locust on next build.

Rémi



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] GIN logging GIN_SEGMENT_UNMODIFIED actions?

[Tom Lane]

Fujii Masao  writes:
> ISTM that the cause of this issue is that gin_desc() uses XLogRecGetData() to
> extract ginxlogVacuumDataLeafPage data from XLOG_GIN_VACUUM_DATA_LEAF_PAGE
> record. Since it's registered by XLogRegisterBufData() in
> ginVacuumPostingTreeLeaf(),
> XLogRecGetBlockData() should be used, instead. Patch attached. Thought?

I think we probably have more issues than that.  See for example 
https://www.postgresql.org/message-id/flat/20160826072658.15676.7628%40wrigleys.postgresql.org

which clearly shows that the replay logic is seeing something wrong too:

2016-08-26 06:01:50 UTC FATAL:  unexpected GIN leaf action: 0
2016-08-26 06:01:50 UTC CONTEXT:  xlog redo Insert item, node:
1663/16387/33108 blkno: 6622 isdata: T isleaf: T 3 segments: 2 (add 0 items)
0 unknown action 0 ???

If it were just a matter of gin_desc() being wrong, we'd not have
gotten such a failure.  (Which is not to say that gin_desc() isn't
wrong; it may well be.)

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] OpenSSL 1.1 breaks configure and more

[Tom Lane]

Heikki Linnakangas  writes:
> Buildfarm animals "locust" and "prairiedog" are not happy with this. 
> They seem to be using OpenSSL 0.9.7, as they failed with errors related 
> to those ECDH calls:

prairiedog definitely is, and since locust is also an ancient OS X
version, that's not too surprising also.  (I imagine gaur/pademelon
will fall over too, next time I turn it on.)

> Tom, Rémi, can you fix locust and prairiedog, please, by updating 
> OpenSSL or removing --with-openssl?

Roger, will do (probably just the latter for today).

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] Fix some corner cases that cube_in rejects

[Tom Lane]

In bug #14300 it's pointed out that cube_in rejects zero-element
cubes, as well as infinity and NaN coordinate values.  Since it's
easy to make such cube values via the cube-from-float-array
constructors, this is a dump/reload hazard.  The attached proposed
patch attempts to fix it up.

To deal with the infinity/NaN issues, I made cube_in and cube_out rely
on float8in_internal and float8out_internal, as we recently did for the
core geometric types.  That causes the response to "1e-700" to be an
out-of-range error rather than silent underflow, which seems to me to
be fine, especially since it removes the platform dependency that's
responsible for needing the separate cube_1.out and cube_3.out files.

I also took the opportunity to make cube_in's error strings and ERRCODE
results match project convention.  This is maybe a bit more debatable,
but I think it's worth doing as long as we're touching the function's
behavior.

I found only one other place that seemed to be assuming that cubes
aren't zero-length, but it would be worth someone reviewing it again
to see if I missed anything.  I'll put this on the commitfest queue.

regards, tom lane

diff --git a/contrib/cube/cube.c b/contrib/cube/cube.c
index 3feddef..9f81ec8 100644
*** a/contrib/cube/cube.c
--- b/contrib/cube/cube.c
*** cube_in(PG_FUNCTION_ARGS)
*** 122,128 
  	cube_scanner_init(str);
  
  	if (cube_yyparse() != 0)
! 		cube_yyerror(, "bogus input");
  
  	cube_scanner_finish();
  
--- 122,128 
  	cube_scanner_init(str);
  
  	if (cube_yyparse() != 0)
! 		cube_yyerror(, "cube parser failed");
  
  	cube_scanner_finish();
  
*** cube_out(PG_FUNCTION_ARGS)
*** 276,302 
  	StringInfoData buf;
  	int			dim = DIM(cube);
  	int			i;
- 	int			ndig;
  
  	initStringInfo();
  
- 	/*
- 	 * Get the number of digits to display.
- 	 */
- 	ndig = DBL_DIG + extra_float_digits;
- 	if (ndig < 1)
- 		ndig = 1;
- 
- 	/*
- 	 * while printing the first (LL) corner, check if it is equal to the
- 	 * second one
- 	 */
  	appendStringInfoChar(, '(');
  	for (i = 0; i < dim; i++)
  	{
  		if (i > 0)
  			appendStringInfoString(, ", ");
! 		appendStringInfo(, "%.*g", ndig, LL_COORD(cube, i));
  	}
  	appendStringInfoChar(, ')');
  
--- 276,290 
  	StringInfoData buf;
  	int			dim = DIM(cube);
  	int			i;
  
  	initStringInfo();
  
  	appendStringInfoChar(, '(');
  	for (i = 0; i < dim; i++)
  	{
  		if (i > 0)
  			appendStringInfoString(, ", ");
! 		appendStringInfoString(, float8out_internal(LL_COORD(cube, i)));
  	}
  	appendStringInfoChar(, ')');
  
*** cube_out(PG_FUNCTION_ARGS)
*** 307,313 
  		{
  			if (i > 0)
  appendStringInfoString(, ", ");
! 			appendStringInfo(, "%.*g", ndig, UR_COORD(cube, i));
  		}
  		appendStringInfoChar(, ')');
  	}
--- 295,301 
  		{
  			if (i > 0)
  appendStringInfoString(, ", ");
! 			appendStringInfoString(, float8out_internal(UR_COORD(cube, i)));
  		}
  		appendStringInfoChar(, ')');
  	}
*** g_cube_distance(PG_FUNCTION_ARGS)
*** 1370,1376 
  	{
  		int			coord = PG_GETARG_INT32(1);
  
! 		if (IS_POINT(cube))
  			retval = cube->x[(coord - 1) % DIM(cube)];
  		else
  			retval = Min(cube->x[(coord - 1) % DIM(cube)],
--- 1358,1366 
  	{
  		int			coord = PG_GETARG_INT32(1);
  
! 		if (DIM(cube) == 0)
! 			retval = 0.0;
! 		else if (IS_POINT(cube))
  			retval = cube->x[(coord - 1) % DIM(cube)];
  		else
  			retval = Min(cube->x[(coord - 1) % DIM(cube)],
diff --git a/contrib/cube/cubedata.h b/contrib/cube/cubedata.h
index 7eaac39..7dd9b11 100644
*** a/contrib/cube/cubedata.h
--- b/contrib/cube/cubedata.h
***
*** 1,5 
--- 1,9 
  /* contrib/cube/cubedata.h */
  
+ /*
+  * This limit is pretty arbitrary, but don't make it so large that you
+  * risk overflow in sizing calculations.
+  */
  #define CUBE_MAX_DIM (100)
  
  typedef struct NDBOX
diff --git a/contrib/cube/cubeparse.y b/contrib/cube/cubeparse.y
index 33606c7..1b65fa9 100644
*** a/contrib/cube/cubeparse.y
--- b/contrib/cube/cubeparse.y
***
*** 4,15 
  /* NdBox = [(lowerleft),(upperright)] */
  /* [(xLL(1)...xLL(N)),(xUR(1)...xUR(n))] */
  
- #define YYSTYPE char *
- #define YYDEBUG 1
- 
  #include "postgres.h"
  
  #include "cubedata.h"
  
  /*
   * Bison doesn't allocate anything that needs to live across parser calls,
--- 4,16 
  /* NdBox = [(lowerleft),(upperright)] */
  /* [(xLL(1)...xLL(N)),(xUR(1)...xUR(n))] */
  
  #include "postgres.h"
  
  #include "cubedata.h"
+ #include "utils/builtins.h"
+ 
+ /* All grammar constructs return strings */
+ #define YYSTYPE char *
  
  /*
   * Bison doesn't allocate anything that needs to live across parser calls,
***
*** 25,33 
  static char *scanbuf;
  static int	scanbuflen;
  
! static int delim_count(char *s, char delim);
! static NDBOX * write_box(unsigned int dim, char *str1, char *str2);
! static NDBOX * write_point_as_box(char *s, int dim);
  
  %}
  
--- 26,34 

Re: [HACKERS] Renaming of pg_xlog and pg_clog

[Bruce Momjian]

On Fri, Aug 26, 2016 at 05:14:36PM +0200, Magnus Hagander wrote:
> On Aug 26, 2016 5:13 PM, "Joshua D. Drake"  wrote:
> >
> > On 08/25/2016 07:39 PM, Michael Paquier wrote:
> >>
> >> Hi all,
> >>
> >> I am relaunching $subject as 10 development will begin soon. As far as
> >> I know, there is agreement that we can do something here. Among the
> >> different proposals I have found:
> >> - pg_clog renamed to pg_commit_status, pg_xact or pg_commit
> >> - pg_xlog renamed to pg_xjournal, pg_wal or pg_journal
> >
> >
> > I think the use of the pg_ prefix is redundant. Just a directory called: wal
> will do (for example).
> >
> > In reference to pg_xlog specifically, it should be wal. It is the Write 
> > Ahead
> Log, not the Journal (although I recognize they can be synonymous). All the
> documentation talks about Write Ahead Log.
> >
> 
> Yes, let's avoid inventing a *third* name for it, please. It's already bad
> enough that we have both wal and xlog.

As far as removing 'pg_', consider that many users place pg_xlog on a
different device, so using "wal" would not be clear it was related to
Postgres. Perhaps we can have initdb --xlogdir use pg_wal, and maybe
document this suggestion.

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

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] OpenSSL 1.1 breaks configure and more

[Heikki Linnakangas]

On 08/29/2016 08:22 PM, Heikki Linnakangas wrote:

On 08/27/2016 05:15 PM, Peter Eisentraut wrote:

On 8/26/16 9:26 PM, Andreas Karlsson wrote:

I have attached a patch which removes the < 0.9.8 compatibility code.
Should we also add a version check to configure? We do not have any such
check currently.


I think that is not necessary.


I was going to change the configure test to check for a different
function that we use, that's only present in 0.9.8 and later. But the
only such functions were related to ECDH, and the use of those functions
is inside "#ifndef OPENSSL_NO_ECDH", so they're not suitable for the
autoconf test. So I gave up. If you try to build with 0.9.7, you'll get
compilation errors because of those ECDH symbols, and with 0.9.6,
probably on some other symbols.

Pushed with some small doc fixes, thanks Andreas! I'll continue
reviewing the rest of the patches.


Buildfarm animals "locust" and "prairiedog" are not happy with this. 
They seem to be using OpenSSL 0.9.7, as they failed with errors related 
to those ECDH calls:


be-secure-openssl.c: In function 'initialize_ecdh':
be-secure-openssl.c:978: error: 'EC_KEY' undeclared (first use in this 
function)
be-secure-openssl.c:978: error: (Each undeclared identifier is reported 
only once

be-secure-openssl.c:978: error: for each function it appears in.)
be-secure-openssl.c:978: error: 'ecdh' undeclared (first use in this 
function)
be-secure-openssl.c:979: warning: ISO C90 forbids mixed declarations and 
code
be-secure-openssl.c:986: warning: implicit declaration of function 
'EC_KEY_new_by_curve_name'
be-secure-openssl.c:991: error: 'SSL_OP_SINGLE_ECDH_USE' undeclared 
(first use in this function)
be-secure-openssl.c:992: warning: implicit declaration of function 
'SSL_CTX_set_tmp_ecdh'
be-secure-openssl.c:993: warning: implicit declaration of function 
'EC_KEY_free'


I only now noticed that Tom said upthread that he still has a buildfarm 
critter using 0.9.7 (that's prairiedog). Sorry for the breakage.


It would be easy to put the version check back to still support 0.9.7, 
most of the changes in this commit was thanks to removing support for 
0.9.6. But that'd complicate the upcoming 1.1.0 support patch slightly, 
so let's stick to the plan and drop the support for <= 0.9.7


Tom, Rémi, can you fix locust and prairiedog, please, by updating 
OpenSSL or removing --with-openssl?


- Heikki



--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] OpenSSL 1.1 breaks configure and more

[Heikki Linnakangas]

On 08/27/2016 05:15 PM, Peter Eisentraut wrote:

On 8/26/16 9:26 PM, Andreas Karlsson wrote:

I have attached a patch which removes the < 0.9.8 compatibility code.
Should we also add a version check to configure? We do not have any such
check currently.


I think that is not necessary.


I was going to change the configure test to check for a different 
function that we use, that's only present in 0.9.8 and later. But the 
only such functions were related to ECDH, and the use of those functions 
is inside "#ifndef OPENSSL_NO_ECDH", so they're not suitable for the 
autoconf test. So I gave up. If you try to build with 0.9.7, you'll get 
compilation errors because of those ECDH symbols, and with 0.9.6, 
probably on some other symbols.


Pushed with some small doc fixes, thanks Andreas! I'll continue 
reviewing the rest of the patches.


- Heikki



--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Renaming of pg_xlog and pg_clog

[Joshua D. Drake]

On 08/29/2016 10:00 AM, Daniel Verite wrote:


Let's imagine that pg_xlog is named wal instead.
How does that help our user with the disk space problem?
Does that point to a path of resolution? I don't see it.
What do we think that user's next move will be?
After all, WAL means Write Ahead *Log*.


If, they look it up (which they would likely have to) they will see it 
isn't removable. :D That is the point I am making. If it is called xlog 
the brain says "logs". If it says wal the brain says, "What is wal?"


At that point they have to look it up and then if they still delete it; 
well that is what emergency rates are for :P


Sincerely,

JD



--
Command Prompt, Inc.  http://the.postgres.company/
+1-503-667-4564
PostgreSQL Centered full stack support, consulting and development.
Everyone appreciates your honesty, until you are honest with them.
Unless otherwise stated, opinions are my own.


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Renaming of pg_xlog and pg_clog

[Daniel Verite]

Joshua D. Drake wrote:

> You log in, see that all the space and you find that you are using a
> ton of disk space. You look around for anything you can delete. You
> find a directory called pg_xlog, it says log, junior ignorant, don't
> want to be a sysadmin 101 says, "delete logs".

Yes, it happens. I don't deny the problem, but I'm wondering
about the wishful thinking we're possibly falling into here
concerning the solution.

Let's imagine that pg_xlog is named wal instead.
How does that help our user with the disk space problem?
Does that point to a path of resolution? I don't see it.
What do we think that user's next move will be?
After all, WAL means Write Ahead *Log*.

On the other hand, by decommissioning pg_xlog as a name,
that makes it obsolete in all presentations, tutorials, docs
that refer to that directory, and there are many of them.
There are years of confusion ahead with questions like
"where is that pg_xlog directory that I'm supposed to
monitor, or move into a different partition, etc...",
for a benefit that remains to be seen.

Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Renaming of pg_xlog and pg_clog

[Robert Haas]

On Sat, Aug 27, 2016 at 2:03 PM, Michael Paquier
 wrote:
> - Rename them, hard break is OK: Michael P, Bruce, Stephen (depends on
> David's input),  Magnus

I'm in favor of this.  But I don't like Peter's proposal to use a more
complicated structure.  As for the new names, how about pg_wal and
pg_xact?  I don't think "pg_trans" is as good; is it transactional or
transient or transport or transitive or what?

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Renaming of pg_xlog and pg_clog

[Fujii Masao]

On Sat, Aug 27, 2016 at 5:33 PM, Michael Paquier
 wrote:
> On Sat, Aug 27, 2016 at 6:34 AM, Andres Freund  wrote:
>> On 2016-08-26 17:31:14 -0400, Peter Eisentraut wrote:
>>> I agree with all that.  But the subject line is specifically about
>>> moving pg_xlog.  So if your opinion is that we shouldn't move pg_xlog,
>>> then that is noted.  But if we were to move it, we can think about a
>>> good place to move it to.
>>
>> I think it's probably worth moving pg_xlog, because the benefit also
>> includes preventing a few users from shooting themselves somewhere
>> vital. That's imo much less the case for some of the other moves.  But I
>> still don't think think a largescale reorganization is a good idea,
>> it'll just stall and nothing will happen.
>
> OK, so let's focus only on the renaming mentioned in $subject. So far
> as I can see on this thread, here are the opinions of people who
> clearly gave one:
> - Rename them, hard break is OK: Michael P, Bruce, Stephen (depends on
> David's input),  Magnus
> - Rename them, hard break not OK: Fujii-san (perhaps do nothing?)
> - Do nothing: Simon (add a README), Tom, Peter E

I vote for "do nothing".
First of all, I can't have much hope for that renaming the directories really
prevents "careless" users from wrongly deleting the important files.

Regards,

-- 
Fujii Masao


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] Send numeric version to clients

[Magnus Hagander]

On Mon, Aug 29, 2016 at 6:37 PM, Robert Haas  wrote:

> On Mon, Aug 29, 2016 at 7:12 PM, Tom Lane  wrote:
> > Craig Ringer  writes:
> >> The same sort of problems apply to network clients, but network
> >> clients don't currently get that option because we only send
> >> server_version on the wire in the startup packet. We don't send
> >> server_version_num.
> >
> >> It'll be immediately useful to have this since clients can test for
> >> it, use it if there, and fall back to their old version parsing code
> >> if there's no server_version_num.
> >
> > I think that this would merely create an attractive nuisance: people
> > would be very likely to omit the "fallback" code and thereby build
> > clients that fail for no very good reason on pre-v10 servers.  As a
> > demonstration that that's not a hypothetical risk, I assert that
> > that's exactly what you propose telling them to do:
> >
> >> Version 10.0 is the perfect time to
> >> do this since many clients will need to update their version handling
> >> anyway, and we can just tell them to use server_version_num now.
>
> You know, I've kind of been on Craig's side of this running dispute up
> until now, but I have to admit that this seems like an awfully good
> argument.  The fact is that nobody's going to be able to rely on
> server_version_num until 9.6 is long gone - which doesn't mean late
> 2021, when official community support will end, but several years
> after that, when most everyone has actually stopped using it.  Of
> course, by that time, assuming we don't backpedal on our decision to
> go with this new versioning scheme, three part version numbers will be
> equally dead and gone, and it's actually a lot easier to extract the
> major version number from the new format than the old.  For example,
> you can just apply atoi() to it:
>
> if (atoi(server_version) < 12)
> fprintf(stderr, "server is ancient, some features will not work\n");
>
> That wouldn't be nearly good enough with three part version numbers
> because you need the second component, as well.  But all that's going
> away now. If you need a port to some other language:
>
> In Perl, you can test int($server_version).
> In Javascript, you can test parseInt(server_version).
> In Python, it's a bit harder.  But int(re.sub(r'[^\d+]+', '',
> server_version)) seems to work.
>

FWIW, a slightly cleaner but still somewhat meh way would be
int(float(server_version)). No need to mess around with regexps and extra
imports.


Long story short, I kind of agree that it might have been better to
> expose server_version_num rather than server_version in the beginning,
> but I'm not sure that it really helps anybody now, especially given
> our decision to simplify the version number format going forward.
>

Yeah, that's a strong point.

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

[HACKERS] Re: [COMMITTERS] pgsql: Change the way that LWLocks for extensions are allocated.

[Robert Haas]

On Sat, Aug 27, 2016 at 3:30 AM, Andres Freund  wrote:
> Hi,
>
> On 2016-02-04 21:43:14 +, Robert Haas wrote:
>> Change the way that LWLocks for extensions are allocated.
>>
>> The previous RequestAddinLWLocks() method had several disadvantages.
>> First, the locks would be in the main tranche; we've recently decided
>> that it's useful for LWLocks used for separate purposes to have
>> separate tranche IDs.  Second, there wasn't any correlation between
>> what code called RequestAddinLWLocks() and what code called
>> LWLockAssign(); when multiple modules are in use, it could become
>> quite difficult to troubleshoot problems where LWLockAssign() ran out
>> of locks.  To fix, create a concept of named LWLock tranches which
>> can be used either by extension or by core code.
>>
>> Amit Kapila and Robert Haas
>
> I noticed that this code has no test coverage:
>
> http://coverage.postgresql.org/src/backend/storage/lmgr/lwlock.c.gcov.html
>
> It'd be good to add some, although I'm not entirely sure what the best
> way is. Maybe add a simple pg_stat_statements test?

That would also have the advantage of improving the test coverage for
pg_stat_statements, which is at the moment quite a bit thinner than
the coverage for lwlock.c.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Renaming of pg_xlog and pg_clog

[Joshua D. Drake]

On 08/29/2016 08:07 AM, Tom Lane wrote:

"Joshua D. Drake"  writes:

Also as a note to the idea that we make break things for external user
space; the next version being v10 is the exact time to do that.


Let's please drop this meme that "v10 is a great time to break things".
We don't want this to be any worse than any other major-version upgrade.


The moment you break backward compatibility, it will be worse. We are 
talking about breaking backward compatibility. So let's just accept it 
as it is, there is a mentality about a major jump. A major jump 
(9.6->10) is *the* perfect time to make world changing, changes.



If we throw thirty different major incompatibilities in at once, we're
going to be hearing about how painful it was for the next decade, even if
any one of them individually would have been manageable.  Or, if we make
the pain factor too high, users will simply not upgrade, and we'll be
faced with demands that we support 9.6 forever.


Whiners always find a reason to whine.

Let's be on two feet here. I am not saying we should jump to using json 
notation for our next release. I am simply stating that any largish 
(even if it is a small patch) changes to expected behavior should be 
done with care. We have a window because no matter how much you yell, I 
yell, Magnus yells, or anybody else yells; the telling story will be, 
"10.0 is a huge jump from 9.6". Most people *WOULD NOT CARE* if we only 
changed one thing. They care that we jumped 4 releases. That type of 
communication implies BIG CHANGES.


Whether you like it or not. Whether I like it or not.

Sincerely,

JD



--
Command Prompt, Inc.  http://the.postgres.company/
+1-503-667-4564
PostgreSQL Centered full stack support, consulting and development.
Everyone appreciates your honesty, until you are honest with them.
Unless otherwise stated, opinions are my own.


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] Send numeric version to clients

[Robert Haas]

On Mon, Aug 29, 2016 at 7:12 PM, Tom Lane  wrote:
> Craig Ringer  writes:
>> The same sort of problems apply to network clients, but network
>> clients don't currently get that option because we only send
>> server_version on the wire in the startup packet. We don't send
>> server_version_num.
>
>> It'll be immediately useful to have this since clients can test for
>> it, use it if there, and fall back to their old version parsing code
>> if there's no server_version_num.
>
> I think that this would merely create an attractive nuisance: people
> would be very likely to omit the "fallback" code and thereby build
> clients that fail for no very good reason on pre-v10 servers.  As a
> demonstration that that's not a hypothetical risk, I assert that
> that's exactly what you propose telling them to do:
>
>> Version 10.0 is the perfect time to
>> do this since many clients will need to update their version handling
>> anyway, and we can just tell them to use server_version_num now.

You know, I've kind of been on Craig's side of this running dispute up
until now, but I have to admit that this seems like an awfully good
argument.  The fact is that nobody's going to be able to rely on
server_version_num until 9.6 is long gone - which doesn't mean late
2021, when official community support will end, but several years
after that, when most everyone has actually stopped using it.  Of
course, by that time, assuming we don't backpedal on our decision to
go with this new versioning scheme, three part version numbers will be
equally dead and gone, and it's actually a lot easier to extract the
major version number from the new format than the old.  For example,
you can just apply atoi() to it:

if (atoi(server_version) < 12)
fprintf(stderr, "server is ancient, some features will not work\n");

That wouldn't be nearly good enough with three part version numbers
because you need the second component, as well.  But all that's going
away now. If you need a port to some other language:

In Perl, you can test int($server_version).
In Javascript, you can test parseInt(server_version).
In Python, it's a bit harder.  But int(re.sub(r'[^\d+]+', '',
server_version)) seems to work.
In Ruby, server_version.to_i seems to do the trick.

Note that these are all one-liners, and I bet the same is true in
mostly other languages.  Even in notoriously verbose languages like
Java or Cobol or ADA it can't be very hard.[1]

If you want the major and minor version numbers, you might need to
write a subroutine for that containing several lines of code ... but
if you're testing for the presence or absence of a feature, that's
irrelevant 99% of the time, and in any event, it's probably 2-3 lines
of code in most.  Note that the C code that implements the version of
PQserverVersion() that handles both old and new style version number
is 33 lines of code, counting variable declarations, comments, and
whitespace.  And approximately half of that is for compatibility with
the old format.

Long story short, I kind of agree that it might have been better to
expose server_version_num rather than server_version in the beginning,
but I'm not sure that it really helps anybody now, especially given
our decision to simplify the version number format going forward.

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

[1] I expect someone to argue (at great length?) that Java should not
be categorized as a notoriously verbose language.


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [COMMITTERS] pgsql: Fix pg_receivexlog --synchronous

[Bruce Momjian]

On Mon, Aug 29, 2016 at 07:34:52AM -0400, Tom Lane wrote:
> Simon Riggs  writes:
> > Fix pg_receivexlog --synchronous
> 
> The buildfarm says you broke the 9.5 branch.
> 
> In general, pushing inessential patches just a few hours before a wrap
> deadline is a dangerous business.  Pushing them without any testing
> is close to irresponsible.

Not being around to fix the breakage after the commit isn't great
either.

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

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Renaming of pg_xlog and pg_clog

[Andres Freund]

On 2016-08-29 12:07:51 -0400, David Steele wrote:
> >> pg_replslot -> pg_tmp/pg_repslot
> > 
> > That's most certainly not ephemeral. Just because something isn't
> > generally appropriate on a standby, doesn't, by far, mean it's ephemeral.
> 
> Yes, ephemeral was a poor choice of words.  I really meant "can be
> excluded from backups".

Then it most certainly can't be called pg_tmp, that'll just invite
people to rm -rf it. And then they'll be screwed.


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Renaming of pg_xlog and pg_clog

[David Steele]

On 8/29/16 11:35 AM, Andres Freund wrote:
> On 2016-08-29 11:18:38 -0400, David Steele wrote:
>> pg_logical -> pg_replgcl
> 
> That seems considerably worse.

Fair enough.  I was just trying to throw something out there to get rid
of the "log" in logical.

>> pg_replslot -> pg_tmp/pg_repslot
> 
> That's most certainly not ephemeral. Just because something isn't
> generally appropriate on a standby, doesn't, by far, mean it's ephemeral.

Yes, ephemeral was a poor choice of words.  I really meant "can be
excluded from backups".

-- 
-David
da...@pgmasters.net


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [WIP] Patches to enable extraction state of query execution from external session

[Oleksandr Shulgin]

On Mon, Aug 29, 2016 at 5:22 PM, maksim  wrote:

> Hi, hackers!
>
> Now I complete extension that provides facility to see the current state
> of query execution working on external session in form of EXPLAIN ANALYZE
> output. This extension works on 9.5 version, for 9.6 and later it doesn't
> support detailed statistics for parallel nodes yet.
>
> I want to present patches to the latest version of PostgreSQL core to
> enable this extension.
>
Hello,

Did you publish the extension itself yet?

Last year (actually, exactly one year ago) I was trying to do something
very similar, and it quickly turned out that signals are not the best way
to make this sort of inspection.  You can find the discussion here:
https://www.postgresql.org/message-id/CACACo5Sz7G0MFauC082iM=xx_hq7qq5ndr4jpo+h-o5vp6i...@mail.gmail.com

Regards.
--
Alex

Re: [HACKERS] [WIP] Patches to enable extraction state of query execution from external session

[Andres Freund]

Hi,

On 2016-08-29 18:22:56 +0300, maksim wrote:
> Now I complete extension that provides facility to see the current state of
> query execution working on external session in form of EXPLAIN ANALYZE
> output. This extension works on 9.5 version, for 9.6 and later it doesn't
> support detailed statistics for parallel nodes yet.

Could you expand a bit on what you want this for exactly?


> 2. Patch that enables to interrupt the query executor
> (executor_hooks.patch).
> This patch enables to hang up hooks on executor function of each node
> (ExecProcNode). I define hooks before any node execution and after
> execution.
> I use this patch to add possibility of query tracing by emitted rows from
> any node. I interrupt query executor after any node delivers one or zero
> rows to upper node. And after execution of specific number trace steps I can
> get the query state of traceable backend which will be somewhat
> deterministic. I use this possibility for regression tests of my extension.

This will increase executor overhead. I think we'll need to find a way
to hide this behind the existing if (instrument) branches.


> 3. Patch that enables to output runtime explain statistics
> (runtime_explain.patch).
> This patch extends the regular explain functionality. The problem is in the
> point that regular explain call makes result output after query execution
> performing InstrEndLoop on nodes where necessary. My patch introduces
> specific flag *runtime* that indicates whether we explain running query and
> does some insertions in source code dedicated to output the statistics of
> running query.

Unless I'm missing something this doesn't really expose a user of this
functionality?


Greetings,

Andres Freund


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [COMMITTERS] pgsql: Fix pg_receivexlog --synchronous

[Tom Lane]

Andres Freund  writes:
> Do we want to revert this until the release, or does somebody want to
> push the fix?

If this had broken the 9.6 branch I would have already summarily
reverted it.  Since it didn't, my only real concern vis-a-vis today's
release is that the build failure in 9.5 calls into question the
quality of the testing that happened in 9.6.  9.6 is still pretty
close to HEAD, but not so close that it's a good idea to push patches
you have not tested in that branch.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

[Andres Freund]

On 2016-08-29 12:56:25 +0300, Heikki Linnakangas wrote:
> On 08/28/2016 12:48 AM, Andres Freund wrote:
> > Attached is a significantly updated patch series (see the mail one up
> > for details about what this is, I don't want to quote it in its
> > entirety).
> > 
> > There's still some corner cases (DISTINCT + SRF, UNION/INTERSECT with
> > SRF) to test / implement and a good bit of code cleanup to do. But
> > feature wise it's pretty much complete.
> 
> Looks good

Thanks for the look!


> aside from the few FIXMEs, TODOs and XXXs

Those I pretty much know to handle.


> DIRTYs.

But I think this one is the "ordering" dependency information, and there
I don't yet have good idea.


> I think we need to come up with a better word for "unsrfify". That's quite a
> mouthful. Perhaps something as boring as "convert_srfs_to_function_rtes".

Yea, that was more of a working title. Maybe implement_targetlist_srfs()?


> Would it make sense for addRangeTableEntryForFunction() to take  a List of
> RangeFunctionElems as argument, now that we have such a struct? The
> lists-of-same-length approach gets a bit tedious.

Yea, I was thinking the same.


> Typos:
> s/fortfour/forfour
> s/Each element of this list a/ Each element of this list is a/

Thanks.

Greetings,

Andres Freund


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Renaming of pg_xlog and pg_clog

[Tom Lane]

Andres Freund  writes:
> On 2016-08-29 11:18:38 -0400, David Steele wrote:
>> pg_replslot -> pg_tmp/pg_repslot

> That's most certainly not ephemeral. Just because something isn't
> generally appropriate on a standby, doesn't, by far, mean it's ephemeral.

Do we need to account for both of those concepts explicitly?

The original point here was to simplify figuring out what needed to be
copied in base backups.  I'm not sure whether we really need a marker
for what's going to be flushed at restart.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [COMMITTERS] pgsql: Fix pg_receivexlog --synchronous

[Andres Freund]

On 2016-08-29 07:34:52 -0400, Tom Lane wrote:
> Simon Riggs  writes:
> > Fix pg_receivexlog --synchronous
> 
> The buildfarm says you broke the 9.5 branch.
> 
> In general, pushing inessential patches just a few hours before a wrap
> deadline is a dangerous business.  Pushing them without any testing
> is close to irresponsible.

And the comment change doesn't actually seem an improvement, because it
makes it harder to understand why a slot forces this to be enabled.

Do we want to revert this until the release, or does somebody want to
push the fix?

Andres


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Renaming of pg_xlog and pg_clog

[Andres Freund]

Hi,

On 2016-08-29 11:18:38 -0400, David Steele wrote:
> pg_logical -> pg_replgcl

That seems considerably worse.

> pg_replslot -> pg_tmp/pg_repslot

That's most certainly not ephemeral. Just because something isn't
generally appropriate on a standby, doesn't, by far, mean it's ephemeral.

Greetings,

Andres Freund


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Renaming of pg_xlog and pg_clog

[David Steele]

On 8/27/16 4:33 AM, Michael Paquier wrote:
> On Sat, Aug 27, 2016 at 6:34 AM, Andres Freund  wrote:
>> On 2016-08-26 17:31:14 -0400, Peter Eisentraut wrote:
>>> I agree with all that.  But the subject line is specifically about
>>> moving pg_xlog.  So if your opinion is that we shouldn't move pg_xlog,
>>> then that is noted.  But if we were to move it, we can think about a
>>> good place to move it to.
>>
>> I think it's probably worth moving pg_xlog, because the benefit also
>> includes preventing a few users from shooting themselves somewhere
>> vital. That's imo much less the case for some of the other moves.  But I
>> still don't think think a largescale reorganization is a good idea,
>> it'll just stall and nothing will happen.
> 
> OK, so let's focus only on the renaming mentioned in $subject. So far
> as I can see on this thread, here are the opinions of people who
> clearly gave one:
> - Rename them, hard break is OK: Michael P, Bruce, Stephen (depends on
> David's input),  Magnus
> - Rename them, hard break not OK: Fujii-san (perhaps do nothing?)
> - Do nothing: Simon (add a README), Tom, Peter E
> 
> As far as I can see, there is a consensus to not rename pg_xlog to
> pg_journal and avoid using a third meaning, but instead use pg_wal. I
> guess that now the other renaming would be pg_clog -> pg_xact. Other
> opinions? Forgot you here?

I'm definitely +1 for a hard break (internal links are a pain).  Ideally
I'd would like to see:

pg_xlog -> pg_wal
pg_clog -> pg_xact
pg_logical -> pg_replgcl

pg_logical is a special case because it contains both ephemeral and
persistent files.  I'd like to see the temporary files in
pg_tmp/pg_replgcl (or whatever it gets renamed to) but that means that
pg_tmp must be on the same device to get atomic renames.  It would be
enough if pg_replgcl had a pgsql_tmp subdirectory so temp files are
excluded from backups with the current logic.

I'm also in favor of leaving configuration files where they are because
these are the files that are most likely to be checked/manipulated in
various user scripts (other than pg_xlog) of course.

As Stephen mentioned I would like to see purely ephemeral data moved
into its own directory.  Maybe $PGDATA/pg_tmp?

pg_subtrans -> pg_tmp/pg_subxact
pg_stat_tamp -> pg_tmp/pg_stat
pg_dynshmem -> pg_tmp/pg_dynshmem (or pg_shmem?)
pg_notify -> pg_tmp/pg_notify
pg_replslot -> pg_tmp/pg_repslot
pg_serial -> pg_tmp/pg_serial
pg_snapshots -> pg_tmp/pg_snapshot

It would be helpful if we had consistent temp directory naming
everywhere so it's clear what to exclude when it is not practical to put
files in the root pg_tmp.  As such I would rename pgsql_tmp to pg_tmp in
base and pg_tblspc/*/*/.

-- 
-David
da...@pgmasters.net


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] [WIP] Patches to enable extraction state of query execution from external session

[maksim]

Hi, hackers!

Now I complete extension that provides facility to see the current state 
of query execution working on external session in form of EXPLAIN 
ANALYZE output. This extension works on 9.5 version, for 9.6 and later 
it doesn't support detailed statistics for parallel nodes yet.


I want to present patches to the latest version of PostgreSQL core to 
enable this extension.



1. Patch that provides facility to send user signal to external backend 
(custom_signals.patch).


This patch transparently extends process signal interface to enable 
sending user defined signals (I call them - custom signals) and defining 
callbacks to them. Formally it will appear in following manner:


/* Register custom signal and define signal callback */

Reason = RegisterCustomProcSignalHandler(sighandler);

void
sighandler(void)
   {
   ...
   }

/* On other session we can send process signal to the first backend */
   SendProcSignal(pid, Reason, backendId)

The InterruptPending flag is set under process signal handling and 
sighandler is called in CHECK_FOR_INTERRUPTS.

I use this patch to notice other backend to send its state to caller.

2. Patch that enables to interrupt the query executor 
(executor_hooks.patch).
This patch enables to hang up hooks on executor function of each node 
(ExecProcNode). I define hooks before any node execution and after 
execution.
I use this patch to add possibility of query tracing by emitted rows 
from any node. I interrupt query executor after any node delivers one or 
zero rows to upper node. And after execution of specific number trace 
steps I can get the query state of traceable backend which will be 
somewhat deterministic. I use this possibility for regression tests of 
my extension.


3. Patch that enables to output runtime explain statistics 
(runtime_explain.patch).
This patch extends the regular explain functionality. The problem is in 
the point that regular explain call makes result output after query 
execution performing InstrEndLoop on nodes where necessary. My patch 
introduces specific flag *runtime* that indicates whether we explain 
running query and does some insertions in source code dedicated to 
output the statistics of running query.
I want to notice that this patch is completed only for 9.5 postgres 
version. For later versions there is not implemented detailed statistics 
for parellel nodes. Now, I'm working at this feature.



That's all. CC welcome!

--
Maksim Milyutin
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company

diff --git a/src/backend/storage/ipc/procsignal.c b/src/backend/storage/ipc/procsignal.c
index a3d6ac5..07270a9 100644
--- a/src/backend/storage/ipc/procsignal.c
+++ b/src/backend/storage/ipc/procsignal.c
@@ -59,12 +59,17 @@ typedef struct
  */
 #define NumProcSignalSlots	(MaxBackends + NUM_AUXPROCTYPES)
 
+static bool CustomSignalPendings[NUM_CUSTOM_PROCSIGNALS];
+static ProcSignalHandler_type CustomHandlers[NUM_CUSTOM_PROCSIGNALS];
+
 static ProcSignalSlot *ProcSignalSlots = NULL;
 static volatile ProcSignalSlot *MyProcSignalSlot = NULL;
 
 static bool CheckProcSignal(ProcSignalReason reason);
 static void CleanupProcSignalState(int status, Datum arg);
 
+static void CustomSignalInterrupt(ProcSignalReason reason);
+
 /*
  * ProcSignalShmemSize
  *		Compute space needed for procsignal's shared memory
@@ -165,6 +170,57 @@ CleanupProcSignalState(int status, Datum arg)
 }
 
 /*
+ * RegisterCustomProcSignalHandler
+ * 		Assign specific handler of custom process signal with new ProcSignalReason key.
+ * 		Return INVALID_PROCSIGNAL if all custom signals have been assigned.
+ */
+ProcSignalReason
+RegisterCustomProcSignalHandler(ProcSignalHandler_type handler)
+{
+	ProcSignalReason reason;
+
+	/* iterate through custom signal keys to find free spot */
+	for (reason = PROCSIG_CUSTOM_1; reason <= PROCSIG_CUSTOM_N; reason++)
+		if (!CustomHandlers[reason - PROCSIG_CUSTOM_1])
+		{
+			CustomHandlers[reason - PROCSIG_CUSTOM_1] = handler;
+			return reason;
+		}
+	return INVALID_PROCSIGNAL;
+}
+
+/*
+ * AssignCustomProcSignalHandler
+ * 		Assign handler of custom process signal with specific ProcSignalReason key.
+ * 		Return old ProcSignal handler.
+ * 		Assume incoming reason is one of custom ProcSignals.
+ */
+ProcSignalHandler_type
+AssignCustomProcSignalHandler(ProcSignalReason reason, ProcSignalHandler_type handler)
+{
+	ProcSignalHandler_type old;
+
+	Assert(reason >= PROCSIG_CUSTOM_1 && reason <= PROCSIG_CUSTOM_N);
+
+	old = CustomHandlers[reason - PROCSIG_CUSTOM_1];
+	CustomHandlers[reason - PROCSIG_CUSTOM_1] = handler;
+	return old;
+}
+
+/*
+ * GetCustomProcSignalHandler
+ * 		Get handler of custom process signal.
+ *		Assume incoming reason is one of custom ProcSignals.
+ */
+ProcSignalHandler_type
+GetCustomProcSignalHandler(ProcSignalReason reason)
+{
+	Assert(reason >= PROCSIG_CUSTOM_1 && reason <= PROCSIG_CUSTOM_N);
+
+	return CustomHandlers[reason - PROCSIG_CUSTOM_1];
+}
+
+/*
  * 

Re: [HACKERS] Renaming of pg_xlog and pg_clog

[Tom Lane]

"Joshua D. Drake"  writes:
> Also as a note to the idea that we make break things for external user 
> space; the next version being v10 is the exact time to do that.

Let's please drop this meme that "v10 is a great time to break things".
We don't want this to be any worse than any other major-version upgrade.
If we throw thirty different major incompatibilities in at once, we're
going to be hearing about how painful it was for the next decade, even if
any one of them individually would have been manageable.  Or, if we make
the pain factor too high, users will simply not upgrade, and we'll be
faced with demands that we support 9.6 forever.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Renaming of pg_xlog and pg_clog

[Joshua D. Drake]

On 08/29/2016 06:42 AM, Daniel Verite wrote:


Aside from that, we might also question how much of the excuse
"pg_xlog looked like it was just deletable logs" is a lie made up
after the fact, because anybody wrecking a database is not against
deflecting a bit of the blame to the software, that's human.

The truth being that they took the gamble that postgres
will somehow recover from the deletion, at the risk of possibly
loosing the latest transactions. That doesn't necessarily look
so bad when current transactions are failing anyway for lack of disk
space, users are yelling at you, and you're frantically searching for
anything to delete in the FS to come back online quickly.
Personally I'm quite skeptical of the *name* of the directory
playing that much of a role in this scenario.


Oh it makes perfect sense.

User who isn't a postgres/database person installs X software. X 
software uses PostgreSQL and you have read on the intertubes about how 
Postgres is awesome. So you install it, get everything up and running 
from the README.md on Github. You walk away.


A year later, after it becomes crucial to whatever it is you do the 
system crashes. You have no idea what is going on, you just set this up 
on some recycled server or VM. You log in, see that all the space and 
you find that you are using a ton of disk space. You look around for 
anything you can delete. You find a directory called pg_xlog, it says 
log, junior ignorant, don't want to be a sysadmin 101 says, "delete logs".


Boom, crushed server. Let us not forget that by far the number of 
PostgreSQL users we have, have never done ANYTHING with postgres except 
make it so something like Drupal can connect to it.


JD






Best regards,




--
Command Prompt, Inc.  http://the.postgres.company/
+1-503-667-4564
PostgreSQL Centered full stack support, consulting and development.
Everyone appreciates your honesty, until you are honest with them.


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Renaming of pg_xlog and pg_clog

[Joshua D. Drake]

On 08/29/2016 12:04 AM, Magnus Hagander wrote:

On Mon, Aug 29, 2016 at 2:45 AM, Jim Nasby > wrote:

On 8/26/16 4:08 PM, Andres Freund wrote:

Splitting of ephemeral data seems to have a benefit, the rest
seems more
like rather noisy busywork to me.


People accidentally blowing away pg_clog or pg_xlog is a pretty
common occurrence, and I don't think there's all that many tools
that reference them. I think it's well worth renaming them.


Pretty sure every single backup tool or script out there is referencing
pg_xlog. If it's not, then it's broken...


No, not really. Consider a filesytem backup using archiving and base 
backups. It doesn't care one lick about pg_xlog. And I guarantee you 
that there are tons of people running a backup like that considering the 
same script would work all the way back to 8.2 (.1?).


Sincerely,

JD

--
Command Prompt, Inc.  http://the.postgres.company/
+1-503-667-4564
PostgreSQL Centered full stack support, consulting and development.
Everyone appreciates your honesty, until you are honest with them.


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Renaming of pg_xlog and pg_clog

[Joshua D. Drake]

On 08/27/2016 11:11 AM, Tom Lane wrote:

Alvaro Herrera  writes:

I'm for renaming too, but I'd go with Peter E's suggestion: move pg_xlog
to something like $PGDATA/var/wal or $PGDATA/srv/wal or something like that.


I think that would make sense if we were to relocate *everything* under
PGDATA into some FHS-like subdirectory structure.  But I'm against moving
the config files for previously-stated reasons, and I doubt it makes sense
to adopt an FHS-like structure only in part.


I think that is a very reasonable suggestion.

Also as a note to the idea that we make break things for external user 
space; the next version being v10 is the exact time to do that.


JD



regards, tom lane





--
Command Prompt, Inc.  http://the.postgres.company/
+1-503-667-4564
PostgreSQL Centered full stack support, consulting and development.
Everyone appreciates your honesty, until you are honest with them.


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Missing checks when malloc returns NULL...

[Tom Lane]

Aleksander Alekseev  writes:
>> if (prodesc->user_proname == NULL || prodesc->internal_proname == NULL)
>> + {
>> +free(prodesc);

> I think that prodesc->user_proname and prodesc->internal_proname should
> also be freed if they are not NULL's.

Hmm, this is kind of putting lipstick on a pig, isn't it?  That code
is still prone to leakage further down, because it calls stuff like
SearchSysCache which is entirely capable of throwing elog(ERROR).

If we're going to touch compile_pltcl_function at all, I'd vote for

(1) changing these malloc calls to MemoryContextAlloc(TopMemoryContext,...

(2) putting the cleanup into a PG_CATCH block, and removing all the
retail free() calls that are there now.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Re: [sqlsmith] FailedAssertion("!(XLogCtl->Insert.exclusiveBackup)", File: "xlog.c", Line: 10200)

[Fujii Masao]

On Mon, Aug 22, 2016 at 10:43 AM, Michael Paquier
 wrote:
> On Sat, Aug 6, 2016 at 6:35 PM, Andreas Seltenreich  
> wrote:
>> Michael Paquier writes:
>>
>>> Andreas, with the patch attached is the assertion still triggered?
>>> [2. text/x-diff; base-backup-crash-v2.patch]
>>
>> I didn't observe the crashes since applying this patch.  There should
>> have been about five by the amount of fuzzing done.
>
> I have reworked the patch, replacing those two booleans with a single
> enum. That's definitely clearer. Also, I have added this patch to the
> CF to not lose track of it:
> https://commitfest.postgresql.org/10/731/
> Horiguchi-san, I have added you as a reviewer as you provided some
> input. I hope you don't mind.

You seem to add another entry for this patch into CommitFest.
Either of them needs to be removed.
https://commitfest.postgresql.org/10/698/

This patch prevents pg_stop_backup from starting while pg_start_backup
is running. OTOH, we also should prevent pg_start_backup from starting
until "previous" pg_stop_backup has completed? What happens if
pg_start_backup starts while pg_stop_backup is running?
As far as I read the current code, ISTM that there is no need to do that,
but it's better to do the double check.

Regards,

-- 
Fujii Masao


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Missing checks when malloc returns NULL...

[Aleksander Alekseev]

> Hello, Michael
> 
> > I don't know how you did it, but this email has visibly broken the
> > original thread. Did you change the topic name?
> 
> I'm very sorry for this. I had no local copy of this thread. So I wrote a
> new email with the same subject hoping it will be OK. Apparently right
> In-Reply-To header is also required. 
> 
> >   if (prodesc->user_proname == NULL || prodesc->internal_proname == NULL)
> > + {
> > +free(prodesc);
> 
> I think that prodesc->user_proname and prodesc->internal_proname should
> also be freed if they are not NULL's.
> 
> > By the way maybe someone knows other procedures besides malloc, realloc
> > and strdup that require special attention?
> 
> I recalled that there is also calloc(). I've found four places that use
> calloc() and look suspicious to me (see attachment). What do you think -
> are these bugs or not?

I've just realized that there is also malloc-compatible ShmemAlloc().
Apparently it's return value sometimes is not properly checked too. See
attachment.

-- 
Best regards,
Aleksander Alekseev
./src/backend/access/transam/slru.c-/* Initialize LWLocks */
./src/backend/access/transam/slru.c:shared->buffer_locks = 
(LWLockPadded *) ShmemAlloc(sizeof(LWLockPadded) * nslots);
./src/backend/access/transam/slru.c-
./src/backend/access/transam/slru.c-Assert(strlen(name) + 1 < 
SLRU_MAX_NAME_LENGTH);
./src/backend/access/transam/slru.c-
strlcpy(shared->lwlock_tranche_name, name, SLRU_MAX_NAME_LENGTH);
./src/backend/access/transam/slru.c-shared->lwlock_tranche_id = 
tranche_id;
./src/backend/access/transam/slru.c-shared->lwlock_tranche.name = 
shared->lwlock_tranche_name;

./src/backend/postmaster/postmaster.c-void
./src/backend/postmaster/postmaster.c-ShmemBackendArrayAllocation(void)
./src/backend/postmaster/postmaster.c-{
./src/backend/postmaster/postmaster.c-  Sizesize = 
ShmemBackendArraySize();
./src/backend/postmaster/postmaster.c-
./src/backend/postmaster/postmaster.c:  ShmemBackendArray = (Backend *) 
ShmemAlloc(size);
./src/backend/postmaster/postmaster.c-  /* Mark all slots as empty */
./src/backend/postmaster/postmaster.c-  memset(ShmemBackendArray, 0, size);
./src/backend/postmaster/postmaster.c-}

./src/backend/storage/ipc/shmem.c-  ShmemVariableCache = (VariableCache)
./src/backend/storage/ipc/shmem.c:  ShmemAlloc(sizeof(*ShmemVariableCache));
./src/backend/storage/ipc/shmem.c-  memset(ShmemVariableCache, 0, 
sizeof(*ShmemVariableCache));
./src/backend/storage/ipc/shmem.c-}

./src/backend/storage/lmgr/lwlock.c-/* Allocate space */
./src/backend/storage/lmgr/lwlock.c:ptr = (char *) 
ShmemAlloc(spaceLocks);
./src/backend/storage/lmgr/lwlock.c-
./src/backend/storage/lmgr/lwlock.c-/* Leave room for dynamic 
allocation of tranches */
./src/backend/storage/lmgr/lwlock.c-ptr += sizeof(int);

./src/backend/storage/lmgr/proc.c:  pgxacts = (PGXACT *) ShmemAlloc(TotalProcs 
* sizeof(PGXACT));
./src/backend/storage/lmgr/proc.c-  MemSet(pgxacts, 0, TotalProcs * 
sizeof(PGXACT));

./src/backend/storage/lmgr/proc.c-  /* Create ProcStructLock spinlock, too */
./src/backend/storage/lmgr/proc.c:  ProcStructLock = (slock_t *) 
ShmemAlloc(sizeof(slock_t));
./src/backend/storage/lmgr/proc.c-  SpinLockInit(ProcStructLock);


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Quorum commit for multiple synchronous replication.

[Fujii Masao]

On Sat, Aug 6, 2016 at 6:36 PM, Petr Jelinek  wrote:
> On 04/08/16 06:40, Masahiko Sawada wrote:
>>
>> On Wed, Aug 3, 2016 at 3:05 PM, Michael Paquier
>>  wrote:
>>>
>>> On Wed, Aug 3, 2016 at 2:52 PM, Masahiko Sawada 
>>> wrote:

 I was thinking that the syntax for quorum method would use '[ ... ]'
 but it will be confused with '( ... )' priority method used.
 001 patch adds 'Any N ( ... )' style syntax but I know that we still
 might need to discuss about better syntax, discussion is very welcome.
 Attached draft patch, please give me feedback.
>>>
>>>
>>> I am +1 for using either "{}" or "[]" to define a quorum set, and -1
>>> for the addition of a keyword in front of the integer defining for how
>>> many nodes server need to wait for.
>>
>>
>> Thank you for reply.
>> "{}" or "[]" are not bad but because these are not intuitive, I
>> thought that it will be hard for uses to use different method for each
>> purpose.
>>
>
> I think the "any" keyword is more explicit and understandable, also closer
> to SQL. So I would be in favor of doing that.

+1

Also I like the following Simon's idea.

https://www.postgresql.org/message-id/canp8+jlhfbvv_pw6grasnupw+bdk5dctu7gwpnap-+-zwvk...@mail.gmail.com
---
* first k (n1, n2, n3) – does the same as k (n1, n2, n3) does now
* any k (n1, n2, n3) – would release waiters as soon as we have the
responses from k out of N standbys. “any k” would be faster, so is
desirable for performance and resilience
---

Regards,

-- 
Fujii Masao


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] Send numeric version to clients

[Tom Lane]

Craig Ringer  writes:
> The same sort of problems apply to network clients, but network
> clients don't currently get that option because we only send
> server_version on the wire in the startup packet. We don't send
> server_version_num.

> It'll be immediately useful to have this since clients can test for
> it, use it if there, and fall back to their old version parsing code
> if there's no server_version_num.

I think that this would merely create an attractive nuisance: people
would be very likely to omit the "fallback" code and thereby build
clients that fail for no very good reason on pre-v10 servers.  As a
demonstration that that's not a hypothetical risk, I assert that
that's exactly what you propose telling them to do:

> Version 10.0 is the perfect time to
> do this since many clients will need to update their version handling
> anyway, and we can just tell them to use server_version_num now.

Sure, it'd be great if we'd done it like this to start with.  But that
ship sailed long ago, and redefining how clients ought to determine
server version at this point is just a bad idea.

I'm also fairly dubious that this is a large problem; surely most
C-coded clients use libpq, for instance, and we already solved this
for them in PQserverVersion.  Or if they aren't using PQserverVersion,
why not?

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Renaming of pg_xlog and pg_clog

[Daniel Verite]

Craig Ringer wrote:

> People won't see a README in amongst 5000 xlog segments while
> freaking out about the sever being down.

Maybe they're more likely to google "pg_xlog".
BTW, renaming it will not help with respect to that, as
there's a pretty good corpus on knowledge linked to that
particular keyword.

The first google results of "pg_xlog" are, for me:

- Solving pg_xlog out of disk space problem on Postgres 
- PostgreSQL: Documentation: 9.1: WAL Internals
- Why is my pg_xlog directory so huge? - PostgreSQL
- Database Soup: Don't delete pg_xlog
...

That's spot-on. For each person deleting stuff in pg_xlog and then
regretting it, how many look it up in the above results and avoid
making the mistake? Will they find comparable results if the
directory is "wal" ?

Aside from that, we might also question how much of the excuse
"pg_xlog looked like it was just deletable logs" is a lie made up
after the fact, because anybody wrecking a database is not against
deflecting a bit of the blame to the software, that's human.

The truth being that they took the gamble that postgres
will somehow recover from the deletion, at the risk of possibly
loosing the latest transactions. That doesn't necessarily look
so bad when current transactions are failing anyway for lack of disk
space, users are yelling at you, and you're frantically searching for
anything to delete in the FS to come back online quickly.
Personally I'm quite skeptical of the *name* of the directory
playing that much of a role in this scenario.

Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Missing checks when malloc returns NULL...

[Aleksander Alekseev]

Hello, Michael

> I don't know how you did it, but this email has visibly broken the
> original thread. Did you change the topic name?

I'm very sorry for this. I had no local copy of this thread. So I wrote a
new email with the same subject hoping it will be OK. Apparently right
In-Reply-To header is also required. 

>   if (prodesc->user_proname == NULL || prodesc->internal_proname == NULL)
> + {
> +free(prodesc);

I think that prodesc->user_proname and prodesc->internal_proname should
also be freed if they are not NULL's.

> By the way maybe someone knows other procedures besides malloc, realloc
> and strdup that require special attention?

I recalled that there is also calloc(). I've found four places that use
calloc() and look suspicious to me (see attachment). What do you think -
are these bugs or not?

-- 
Best regards,
Aleksander Alekseev
Looks like resource leak:

./src/backend/storage/buffer/localbuf.c:LocalBufferDescriptors = 
(BufferDesc *) calloc(nbufs, sizeof(BufferDesc));
./src/backend/storage/buffer/localbuf.c:LocalBufferBlockPointers = (Block 
*) calloc(nbufs, sizeof(Block));
./src/backend/storage/buffer/localbuf.c:LocalRefCount = (int32 *) 
calloc(nbufs, sizeof(int32));
./src/backend/storage/buffer/localbuf.c-if (!LocalBufferDescriptors || 
!LocalBufferBlockPointers || !LocalRefCount)
./src/backend/storage/buffer/localbuf.c-ereport(FATAL,
./src/backend/storage/buffer/localbuf.c-
(errcode(ERRCODE_OUT_OF_MEMORY),
./src/backend/storage/buffer/localbuf.c- errmsg("out of 
memory")));
./src/backend/storage/buffer/localbuf.c-

./src/interfaces/libpq/fe-print.c-  nTups = PQntuples(res);
./src/interfaces/libpq/fe-print.c:  if (!(fieldNames = (const char **) 
calloc(nFields, sizeof(char *
./src/interfaces/libpq/fe-print.c-  {
./src/interfaces/libpq/fe-print.c-  fprintf(stderr, libpq_gettext("out 
of memory\n"));
./src/interfaces/libpq/fe-print.c-  abort();
./src/interfaces/libpq/fe-print.c-  }
./src/interfaces/libpq/fe-print.c:  if (!(fieldNotNum = (unsigned char *) 
calloc(nFields, 1)))
./src/interfaces/libpq/fe-print.c-  {
./src/interfaces/libpq/fe-print.c-  fprintf(stderr, libpq_gettext("out 
of memory\n"));
./src/interfaces/libpq/fe-print.c-  abort();
./src/interfaces/libpq/fe-print.c-  }
./src/interfaces/libpq/fe-print.c:  if (!(fieldMax = (int *) 
calloc(nFields, sizeof(int
./src/interfaces/libpq/fe-print.c-  {
./src/interfaces/libpq/fe-print.c-  fprintf(stderr, libpq_gettext("out 
of memory\n"));
./src/interfaces/libpq/fe-print.c-  abort();
./src/interfaces/libpq/fe-print.c-  }


Two identical pieces of code:

./src/backend/bootstrap/bootstrap.c-scan = 
heap_beginscan_catalog(rel, 0, NULL);
./src/backend/bootstrap/bootstrap.c-i = 0;
./src/backend/bootstrap/bootstrap.c-while ((tup = 
heap_getnext(scan, ForwardScanDirection)) != NULL)
./src/backend/bootstrap/bootstrap.c-++i;
./src/backend/bootstrap/bootstrap.c-heap_endscan(scan);
./src/backend/bootstrap/bootstrap.c:app = Typ = ALLOC(struct typmap 
*, i + 1);
./src/backend/bootstrap/bootstrap.c-while (i-- > 0)
./src/backend/bootstrap/bootstrap.c:*app++ = ALLOC(struct 
typmap, 1);
./src/backend/bootstrap/bootstrap.c-*app = NULL;
./src/backend/bootstrap/bootstrap.c-scan = 
heap_beginscan_catalog(rel, 0, NULL);
./src/backend/bootstrap/bootstrap.c-app = Typ;
./src/backend/bootstrap/bootstrap.c-while ((tup = 
heap_getnext(scan, ForwardScanDirection)) != NULL)
./src/backend/bootstrap/bootstrap.c-{


./src/backend/bootstrap/bootstrap.c-scan = 
heap_beginscan_catalog(rel, 0, NULL);
./src/backend/bootstrap/bootstrap.c-i = 0;
./src/backend/bootstrap/bootstrap.c-while ((tup = 
heap_getnext(scan, ForwardScanDirection)) != NULL)
./src/backend/bootstrap/bootstrap.c-++i;
./src/backend/bootstrap/bootstrap.c-heap_endscan(scan);
./src/backend/bootstrap/bootstrap.c:app = Typ = ALLOC(struct typmap 
*, i + 1);
./src/backend/bootstrap/bootstrap.c-while (i-- > 0)
./src/backend/bootstrap/bootstrap.c:*app++ = ALLOC(struct 
typmap, 1);
./src/backend/bootstrap/bootstrap.c-*app = NULL;
./src/backend/bootstrap/bootstrap.c-scan = 
heap_beginscan_catalog(rel, 0, NULL);
./src/backend/bootstrap/bootstrap.c-app = Typ;
./src/backend/bootstrap/bootstrap.c-while ((tup = 
heap_getnext(scan, ForwardScanDirection)) != NULL)
./src/backend/bootstrap/bootstrap.c-{

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Renaming of pg_xlog and pg_clog

[Craig Ringer]

On 29 August 2016 at 20:28, Michael Paquier  wrote:
> On Mon, Aug 29, 2016 at 5:28 PM, Craig Ringer
>  wrote:
>> On 29 August 2016 at 14:30, Michael Paquier  
>> wrote:
>>> On Mon, Aug 29, 2016 at 2:36 PM, Craig Ringer
>>>  wrote:
 I don't care if it comes as part of some greater reorg or not but I'll be
 really annoyed if scope creep lands up killing the original proposal to 
 just
 rename these dirs. I think that a simple rename should be done first. Then
 if some greater reorg is to be done it can be done shortly after. The only
 people that'll upset are folks tracking early 10.0 dev and they'll be aware
 it's coming.
>>>
>>> Okay, so let's do it. Attached are two patches:
>>> - 0001 renames pg_clog to pg_trans. I have let clog.c with its current
>>> name, as well as its structures. That's the mechanical patch, the ony
>>> interesting part being in pg_upgrade.
>>> - 0002 renames pg_xlog to pg_wal.
>>
>> Is there any expectation that a 10.0 pg_basebackup should work on a
>> 9.x server, or fail to work gracefully? There doesn't look to be any
>> version specific handling of the rename there.
>
> Oops. Per the docs:
> pg_basebackup works with servers of the same or an older major
> version, down to 9.1. However, WAL streaming mode (-X stream) only
> works with server version 9.3 and later, and tar format mode
> (--format=tar) of the current version only works with server version
> 9.5 or later.
>
> So we need to do a bit better than what the patch proposes, but that's
> actually just tweaking the things with pg_xlog/pg_wal depending on the
> version of the target server.

Cool. I'll mark as waiting on author pending that.

It'll be good to get this footgun put away.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Renaming of pg_xlog and pg_clog

[Michael Paquier]

On Mon, Aug 29, 2016 at 5:28 PM, Craig Ringer
 wrote:
> On 29 August 2016 at 14:30, Michael Paquier  wrote:
>> On Mon, Aug 29, 2016 at 2:36 PM, Craig Ringer
>>  wrote:
>>> I don't care if it comes as part of some greater reorg or not but I'll be
>>> really annoyed if scope creep lands up killing the original proposal to just
>>> rename these dirs. I think that a simple rename should be done first. Then
>>> if some greater reorg is to be done it can be done shortly after. The only
>>> people that'll upset are folks tracking early 10.0 dev and they'll be aware
>>> it's coming.
>>
>> Okay, so let's do it. Attached are two patches:
>> - 0001 renames pg_clog to pg_trans. I have let clog.c with its current
>> name, as well as its structures. That's the mechanical patch, the ony
>> interesting part being in pg_upgrade.
>> - 0002 renames pg_xlog to pg_wal.
>
> Is there any expectation that a 10.0 pg_basebackup should work on a
> 9.x server, or fail to work gracefully? There doesn't look to be any
> version specific handling of the rename there.

Oops. Per the docs:
pg_basebackup works with servers of the same or an older major
version, down to 9.1. However, WAL streaming mode (-X stream) only
works with server version 9.3 and later, and tar format mode
(--format=tar) of the current version only works with server version
9.5 or later.

So we need to do a bit better than what the patch proposes, but that's
actually just tweaking the things with pg_xlog/pg_wal depending on the
version of the target server.

> The patch does not update the translations. I wonder if it's worth
> doing so to save translators the hassle by sed'ing the following
> lines:
> sed -i 's/\/pg_wal/g' src/backend/po/*.po
> ?

Yes I was wondering about that... But concluded that normally the
translation updates would just do it. It is not complicated to update
if need be anyway.

> src/backend/access/transam/README should probably have a note about the 
> rename.

Good idea.

> Looks like changes in pg_upgrade for clog are a bit more than the
> mechanical changes elsewhere, but seem sensible to me. I haven't yet
> done a test pg_upgrade run.

I have tested that FWIW using 10devel -> 10devel, 9.5 -> 10devel, 9.4
-> 10devel. There are versions of 10devel using pg_xlog and others
pg_wal if this patch is introduced. I am aware of the fact that
pg_upgrade supports upgrades for the same major version though it
seems to me that we would not want to support this scenario, which is
why it would be good to get this patch at the beginning of the dev
cycle.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [COMMITTERS] pgsql: Fix pg_receivexlog --synchronous

[Michael Paquier]

On Mon, Aug 29, 2016 at 8:34 PM, Tom Lane  wrote:
> Simon Riggs  writes:
>> Fix pg_receivexlog --synchronous
>
> The buildfarm says you broke the 9.5 branch.
>
> In general, pushing inessential patches just a few hours before a wrap
> deadline is a dangerous business.  Pushing them without any testing
> is close to irresponsible.

This area of the code has faced some refactoring from Magnus lately,
so you need this on REL9_5_STABLE:
--- a/src/bin/pg_basebackup/receivelog.c
+++ b/src/bin/pg_basebackup/receivelog.c
@@ -534,7 +534,7 @@ ReceiveXlogStream(PGconn *conn, XLogRecPtr
startpos, uint32 timeline,
}
else
{
-   if (stream->synchronous)
+   if (synchronous)
reportFlushPosition = true;
else
reportFlushPosition = false;
-- 
Michael


fix-receivexlog-95.patch
Description: application/download

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Declarative partitioning - another take

[Robert Haas]

On Fri, Aug 26, 2016 at 1:33 PM, Amit Langote
 wrote:
> We do take a lock on the parent because we would be changing its partition
> descriptor (relcache).  I changed MergeAttributes() such that an
> AccessExclusiveLock instead of ShareUpdateExclusiveLock is taken if the
> parent is a partitioned table.

Hmm, that seems both good and bad.  On the good side, as mentioned,
being able to rely on the partition descriptor not changing under us
makes this sort of thing much easier to reason about.  On the bad
side, it isn't good for this feature to have worse concurrency than
regular inheritance.  Not sure what to do about this.

> If we need an AccessExclusiveLock on parent to add/remove a partition
> (IOW, changing that child table's partitioning information), then do we
> need to lock the individual partitions when reading partition's
> information?  I mean to ask why the simple syscache look-ups to get each
> partition's bound wouldn't do.

Well, if X can't be changed without having an AccessExclusiveLock on
the parent, then an AccessShareLock on the parent is sufficient to
read X, right?  Because those lock modes conflict.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [COMMITTERS] pgsql: Fix pg_receivexlog --synchronous

[Tom Lane]

Simon Riggs  writes:
> Fix pg_receivexlog --synchronous

The buildfarm says you broke the 9.5 branch.

In general, pushing inessential patches just a few hours before a wrap
deadline is a dangerous business.  Pushing them without any testing
is close to irresponsible.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pg_receivexlog does not report flush position with --synchronous

[Simon Riggs]

On 23 August 2016 at 14:57, Michael Paquier  wrote:
> On Tue, Aug 23, 2016 at 9:48 PM, Gabriele Bartolini
>  wrote:
>> Hi Simon and Michael,
>>
>> 2016-08-23 10:39 GMT+02:00 Simon Riggs :
>>>
>>> Agreed, but I'd move all the comments above the block.
>>
>>
>> That's fine with me.
>
> +1.

Committed and backpatched to 9.5. Thank you both.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] [PATCH] Send numeric version to clients

[Craig Ringer]

Hi all

It's recently been observed[1] that the 10.0 version scheme change has
affected PostGIS, which relies on parsing the server version string
and broke when faced with a string like "10.0devel" since it expected
a minor version.

In that thread Tom points out [2] that they should be using
PG_VERSION_NUM from the Makefile, or PG_CATALOG_VERSION from the
headers.

The same sort of problems apply to network clients, but network
clients don't currently get that option because we only send
server_version on the wire in the startup packet. We don't send
server_version_num.

It'll be immediately useful to have this since clients can test for
it, use it if there, and fall back to their old version parsing code
if there's no server_version_num. Version 10.0 is the perfect time to
do this since many clients will need to update their version handling
anyway, and we can just tell them to use server_version_num now.

I'm a PgJDBC committer (albeit largely inactive lately), so I'm
thoroughly familiar with at least one client, and I'd really like to
be able to have PgJDBC able to prefer server_version_num. That's how I
originally started looking at this. Also, clients relying on
server_version makes configure's --with-extra-version nearly unusable
in practice since if you build Pg 9.4.9-mypatch a bunch of clients
fall over, as I discovered when working on BDR.

I'm not convinced by the cost concerns that originally caused it not
to be made GUC_REPORT [3], or at least that they still apply today.
Since that change 10 years ago networks have changed a lot. More
significantly we've since added both IntervalStyle ([4], df7641e2, Ron
Mayer / Tom) and application_name ([5], 59ed94ad, Marko Kreen / Tom)
as GUC_REPORT params.

The startup packet is 331 bytes on my system, with a short
application_name 'psql', short username 'craig', etc. Adding
server_version_num would push it up ~26 bytes to ~357, a 7% increase
on a packet we send once at connection establishment. Given that
network performance is overwhelmingly dominated by round-trip costs
even on fast local networks this is going to be practically
undetectable. A minimal connect-and-disconnect psql session with no
SSL exchanges 14 packets of 1363 bytes (TCP level), of which 670 bytes
are server -> client. So that's a 4% increase on the network size of
the most utterly trivial conversation possible, with zero new packets
and zero new round trips.

Unsurprisingly the pgbench effects are undetectable.

Compare that to the effects of [6] pipelining work on the protocol,
where I got speedups of 300x or more by tackling round trip costs.

Can we please send server_version_num on the wire for 10.0? Patches attached.

(BTW, I noticed while prepping that patch that we have identically
duplicated docs for GUC_REPORT params in protocol.sgml and
libpq.sgml.)

[1] 
https://www.postgresql.org/message-id/01d2014c$f84b9190$e8e2b4b0$@pcorp.us
[2] https://www.postgresql.org/message-id/1585.1472410...@sss.pgh.pa.us
[3] https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=e35ea51
[4] https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=df7641e2
[5] https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=59ed94ad
[6] https://commitfest.postgresql.org/10/634/

-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services
From 818fdc00c0f0f9d98082830c4e9fdc40e9083859 Mon Sep 17 00:00:00 2001
From: Craig Ringer 
Date: Mon, 29 Aug 2016 11:31:52 +0800
Subject: [PATCH 1/2] Report server_version_num alongside server_version in
 startup packet

We expose PG_VERSION_NUM in Makefiles and headers and in pg_settings,
but the equivalent server_version_num isn't sent in the startup packet
so clients must rely on parsing the server_version. Make
server_version_num GUC_REPORT so clients can use server_version_num if
present and fall back to server_version for older PostgreSQL versions.
---
 doc/src/sgml/libpq.sgml  | 6 --
 doc/src/sgml/protocol.sgml   | 4 +++-
 src/backend/utils/misc/guc.c | 2 +-
 3 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index 2f9350b..5428282 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -1632,6 +1632,7 @@ const char *PQparameterStatus(const PGconn *conn, const char *paramName);
   
Parameters reported as of the current release include
server_version,
+   server_version_num,
server_encoding,
client_encoding,
application_name,
@@ -1647,9 +1648,10 @@ const char *PQparameterStatus(const PGconn *conn, const char *paramName);
standard_conforming_strings was not reported by releases
before 8.1;
IntervalStyle was not reported by releases before 8.4;
-   application_name was not reported by releases before 9.0.)
+   application_name was not reported by releases before 9.0;
+   server_version_num 

Re: [HACKERS] Sample configuration files

[Amit Kapila]

On Mon, Aug 29, 2016 at 7:04 AM, Vik Fearing  wrote:
> We have sample configuration files for postgresql.conf and
> recovery.conf, but we do not have them for contrib modules.  This patch
> attempts to add them.
>
> Although the patch puts the sample configuration files in the tree, it
> doesn't try to install them.  That's partly because I think it would
> need an extension version bump and that doesn't seem worth it.
>

What is the use case and how these files suppose to work?  Do you
expect these to be loaded as we do for postgresql.conf?


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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

[Heikki Linnakangas]

On 08/28/2016 12:48 AM, Andres Freund wrote:

Attached is a significantly updated patch series (see the mail one up
for details about what this is, I don't want to quote it in its
entirety).

There's still some corner cases (DISTINCT + SRF, UNION/INTERSECT with
SRF) to test / implement and a good bit of code cleanup to do. But
feature wise it's pretty much complete.


Looks good, aside from the few FIXMEs, TODOs and XXXs and DIRTYs.

I think we need to come up with a better word for "unsrfify". That's 
quite a mouthful. Perhaps something as boring as 
"convert_srfs_to_function_rtes".


Would it make sense for addRangeTableEntryForFunction() to take  a List 
of RangeFunctionElems as argument, now that we have such a struct? The 
lists-of-same-length approach gets a bit tedious.


Typos:
s/fortfour/forfour
s/Each element of this list a/ Each element of this list is a/

- Heikki



--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Renaming of pg_xlog and pg_clog

[Petr Jelinek]

On 27/08/16 18:50, Tom Lane wrote:

Michael Paquier  writes:

OK, so let's focus only on the renaming mentioned in $subject. So far
as I can see on this thread, here are the opinions of people who
clearly gave one:
- Rename them, hard break is OK: Michael P, Bruce, Stephen (depends on
David's input),  Magnus
- Rename them, hard break not OK: Fujii-san (perhaps do nothing?)
- Do nothing: Simon (add a README), Tom, Peter E


I'm against moving/renaming the
configuration files, because I think that will break a lot of users'
scripts and habits without really buying much.


I don't agree with this part, mainly because there is significant number 
of installations (everything that uses debian/ubuntu scripts) that 
already don't have configuration files inside the data directory.


On the other matters:
+1 for renaming pg_xlog to pg_wal and pg_clog to pg_xact/trans (don't 
care really which one)


And +1 for renaming pg_logical to something more reasonable.

--
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] GIN logging GIN_SEGMENT_UNMODIFIED actions?

[Fujii Masao]

On Fri, Aug 26, 2016 at 11:35 PM, Fujii Masao  wrote:
> On Tue, May 10, 2016 at 9:57 PM, Alexander Korotkov
>  wrote:
>> Hi!
>>
>> On Mon, May 9, 2016 at 10:46 PM, Andres Freund  wrote:
>>>
>>> trying to debug something I saw the following in pg_xlogdump output:
>>>
>>> rmgr: Gin len (rec/tot):  0/   274, tx:  0, lsn:
>>> 1C/DF28AEB0, prev 1C/DF289858, desc: VACUUM_DATA_LEAF_PAGE  3 segments: 5
>>> unknown action 0 ???, blkref #0: rel 1663/16384/16435 blk 310982
>>>
>>> note the "segments: 5 unknown action 0 ???" bit.  That doesn't seem
>>> right, given:
>>> #define GIN_SEGMENT_UNMODIFIED  0   /* no action (not used in
>>> WAL records) */
>>
>>
>> I've checked GIN code.  Have no idea of how such wal record could be
>> generated...
>
> I encountered the same issue when executing the following queries and
> running pg_xlogdump.

ISTM that the cause of this issue is that gin_desc() uses XLogRecGetData() to
extract ginxlogVacuumDataLeafPage data from XLOG_GIN_VACUUM_DATA_LEAF_PAGE
record. Since it's registered by XLogRegisterBufData() in
ginVacuumPostingTreeLeaf(),
XLogRecGetBlockData() should be used, instead. Patch attached. Thought?

BTW, in REDO side, ginRedoVacuumDataLeafPage() uses XLogRecGetBlockData()
to extract ginxlogVacuumDataLeafPage data.

Regards,

-- 
Fujii Masao
*** a/src/backend/access/rmgrdesc/gindesc.c
--- b/src/backend/access/rmgrdesc/gindesc.c
***
*** 144,150  gin_desc(StringInfo buf, XLogReaderState *record)
  			break;
  		case XLOG_GIN_VACUUM_DATA_LEAF_PAGE:
  			{
! ginxlogVacuumDataLeafPage *xlrec = (ginxlogVacuumDataLeafPage *) rec;
  
  if (XLogRecHasBlockImage(record, 0))
  	appendStringInfoString(buf, " (full page image)");
--- 144,151 
  			break;
  		case XLOG_GIN_VACUUM_DATA_LEAF_PAGE:
  			{
! ginxlogVacuumDataLeafPage *xlrec =
! 	(ginxlogVacuumDataLeafPage *) XLogRecGetBlockData(record, 0, NULL);
  
  if (XLogRecHasBlockImage(record, 0))
  	appendStringInfoString(buf, " (full page image)");

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] PostgreSQL Version 10, missing minor version

[Craig Ringer]

On 29 August 2016 at 11:46, Andres Freund  wrote:
> On 2016-08-29 11:41:00 +0800, Craig Ringer wrote:
>> On 29 August 2016 at 02:52, Tom Lane  wrote:
>> > "Regina Obe"  writes:
>> >> The routine in PostGIS to parse out the version number from pg_config is
>> >> breaking in the 10 cycle
>> >
>> > TBH, I wonder why you are doing that in the first place; it does not
>> > seem like the most reliable source of version information.  If you
>> > need to do compile-time tests, PG_CATALOG_VERSION is considered the
>> > best thing to look at, or VERSION_NUM in Makefiles.
>>
>> This is my cue to pop up and say that if you're looking at the startup
>> message you have to use the version string, despite it not being the
>> most reliable source of information, because we don't send
>> server_version_num  ;)
>>
>> Patch attached. Yes, I know PostGIS doesn't use it, but it makes no
>> sense to tell people not to parse the server version out in some
>> situations then force them to in others.
>
> If they're using pg_config atm, that seems unlikely to be related. That
> sounds more like a build time issue - there won't be a running server.

Yeah, you're right, and I shouldn't hijack an unrelated thread. Please
disregard, will follow up separately.



-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Renaming of pg_xlog and pg_clog

[Craig Ringer]

On 29 August 2016 at 14:30, Michael Paquier  wrote:
> On Mon, Aug 29, 2016 at 2:36 PM, Craig Ringer
>  wrote:
>> I don't care if it comes as part of some greater reorg or not but I'll be
>> really annoyed if scope creep lands up killing the original proposal to just
>> rename these dirs. I think that a simple rename should be done first. Then
>> if some greater reorg is to be done it can be done shortly after. The only
>> people that'll upset are folks tracking early 10.0 dev and they'll be aware
>> it's coming.
>
> Okay, so let's do it. Attached are two patches:
> - 0001 renames pg_clog to pg_trans. I have let clog.c with its current
> name, as well as its structures. That's the mechanical patch, the ony
> interesting part being in pg_upgrade.
> - 0002 renames pg_xlog to pg_wal.

Is there any expectation that a 10.0 pg_basebackup should work on a
9.x server, or fail to work gracefully? There doesn't look to be any
version specific handling of the rename there.

Otherwise looks good.

Doesn't upset 'make check' or the TAP recovery suite.

Works:

src/test/regress/tmp_check/data $ ls
basepg_commit_ts  pg_hba.confpg_logicalpg_notify
pg_serial pg_stat  pg_subtrans  pg_trans PG_VERSION
postgresql.auto.conf  postmaster.opts
global  pg_dynshmem   pg_ident.conf  pg_multixact  pg_replslot
pg_snapshots  pg_stat_tmp  pg_tblspcpg_twophase  pg_wal
postgresql.conf

It leaves pg_xlogfilename etc with original names as it IMO should.
There's no need to break more than we have to and it's still the xlog.

The documentation on backup/restore might benefit from a note saying
that pg_wal was named pg_xlog prior to 10.0 so tools intended to work
on older versions should check PG_VERSION. Though in practice most
people who write new tools will target 10.0+ and people maintaining
older tools will know, so it's not a big deal.

I don't know if the renaming in XLogFileRead of XLOG_FROM_PG_XLOG =>
XLOG_FROM_PG_WAL  is really necessary, but tend to think it's good
since that define explicitly refers to the directory name, not
transaction logs in general.

The patch does not update the translations. I wonder if it's worth
doing so to save translators the hassle by sed'ing the following
lines:

-errhint("The database server
will regularly poll the pg_xlog subdirectory to check for files placed
there.")));
+errhint("The database server
will regularly poll the pg_wal subdirectory to check for files placed
there.")));

-(errmsg("could not open directory
\"%s\": %m", "pg_xlog")));
+(errmsg("could not open directory
\"%s\": %m", "pg_wal")));

with

sed -i 's/\/pg_wal/g' src/backend/po/*.po

?



src/backend/access/transam/README should probably have a note about the rename.

Looks like changes in pg_upgrade for clog are a bit more than the
mechanical changes elsewhere, but seem sensible to me. I haven't yet
done a test pg_upgrade run.



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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Why is a newly created index contains the invalid LSN?

[Yury Zhuravlev]

Jim Nasby wrote:
Yeah, especially since you mentioned this being for backups. I 
suspect you *want* those WAL records marked with 0, because that 
tells you that you can't rely on WAL when you back that data up.


Thanks, you right if you doing incremental backup you try compare every 
page LSN with last backup LSN. For my page tracking system (ptrack) it is 
secondary cheks but for classic pg_arman algorithm it is main approach. 
If Invalid LSN will be realy sign of broken page header it help for 
third-party applications. 



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


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] Transaction traceability - txid_status(bigint)

[Craig Ringer]

On 29 August 2016 at 11:45, Andres Freund  wrote:
> Hi,
>
> On 2016-08-29 11:25:39 +0800, Craig Ringer wrote:
>>   ERROR:  could not access status of transaction 778793573
>>   DETAIL:  could not open file "pg_clog/02E6": No such file or directory
>>
>> What I'd really like is to be able to ask transam.c to handle the
>> xid_in_recent_past logic, treating an attempt to read an xid from
>> beyond the clog truncation threshold as a soft error indicating
>> unknown xact state. But that involves delving into slru.c, and I
>> really, really don't want to touch that for what should be a simple
>> and pretty noninvasive utility function.
>
> Can't you "just" check this against ShmemVariableCache->oldestXid while
> holding appropriate locks?

Hm. Yeah, I should've thought of that. Thank you.

>> A PG_TRY to trap ERRCODE_UNDEFINED_FILE seems like it'd be sufficient,
>> except for two issues:
>
> It seems like a bad idea to PG_CATCH and not re-throw an error. That
> generally is quite error prone. At the very least locking and such gets
> a lot more complicated (as you noticed below).

Yeah, and as I remember from the "fun" of trying to write apply errors
to tables in BDR. It wasn't my first choice.

>> * TransactionIdGetStatus() releases the CLogControlLock taken by
>> SimpleLruReadPage_ReadOnly() on normal exit but not after an exception
>> thrown from SlruReportIOError(). It seems appropriate for
>> SimpleLruReadPage() to release the LWLock before calling
>> SlruReportIOError(), so I've done that as a separate patch (now 0001).
>
> We normally prefer to handle this via the "bulk" releases in the error
> handlers. It's otherwise hard to write code that handles these
> situations reliably. It's different for spinlocks, but those normally
> protect far smaller regions of code.

Fair enough. It's not a complex path, but there are a _lot_ of
callers, and while I can't really imagine any of them relying on the
CLogControLock being held on error it's not something I was keen to
change. I thought complicating the clog with a soft-error interface
was worse and didn't come up with a better approach.

Said better approach attached in revised series. Thanks.

My only real complaint with doing this is that it's a bit more
conservative. But in practice clog truncation probably won't follow
that far behind oldestXmin so except in fairly contrived circumstances
it won't hurt. Apps that need guarantees about how old an xid they can
get status on can hold down xmin with a replication slot, a dummy
prepared xact, or whatever. If we find that becomes a common need that
should be made simpler then appropriate API to allow apps to hold down
clog truncation w/o blocking vacuuming can be added down the track.

-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services
From b69f99b63f667e745ccdee2130ee1b50690109d4 Mon Sep 17 00:00:00 2001
From: Craig Ringer 
Date: Fri, 19 Aug 2016 14:44:15 +0800
Subject: [PATCH 1/3] Introduce txid_status(bigint) to get status of an xact

If an appliation is disconnected while a COMMIT request is in flight,
the backend crashes mid-commit, etc, then an application may not be
sure whether or not a commit completed successfully or was rolled
back. While two-phase commit solves this it does so at a considerable
overhead, so introduce a lighter alternative.

txid_status(bigint) lets an application determine the status of a
a commit based on an xid-with-epoch as returned by txid_current()
or similar. Status may be committed, aborted, in-progress (including
prepared xacts) or null if the xact is too old for its commit status
to still be retained because it has passed the wrap-around epoch
boundary.

Applications must call txid_current() in their transactions to make
much use of this since PostgreSQL does not automatically report an xid
to the client when one is assigned.
---
 doc/src/sgml/func.sgml |  31 ++
 src/backend/access/transam/clog.c  |  23 ---
 src/backend/utils/adt/txid.c   | 119 +
 src/include/access/clog.h  |  23 +++
 src/include/catalog/pg_proc.h  |   2 +
 src/include/utils/builtins.h   |   1 +
 src/test/regress/expected/txid.out |  68 +
 src/test/regress/sql/txid.sql  |  38 
 8 files changed, 282 insertions(+), 23 deletions(-)

diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 5148095..d8b086f 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -17143,6 +17143,10 @@ SELECT collation for ('foo' COLLATE "de_DE");
 txid_visible_in_snapshot

 
+   
+txid_status
+   
+

 The functions shown in 
 provide server transaction information in an exportable form.  The main
@@ -17193,6 +17197,11 @@ SELECT collation for ('foo' COLLATE "de_DE");
boolean
is transaction ID visible in snapshot? (do not use with 

Re: [HACKERS] pg_dump with tables created in schemas created by extensions

[Michael Paquier]

On Sat, Aug 27, 2016 at 8:15 AM, Tomas Vondra
 wrote:
> On 08/27/2016 12:37 AM, Tom Lane wrote:
>> =?UTF-8?B?TWFydMOtbiBNYXJxdcOpcw==?=  writes:
>>> Looking at this issue today, I found that we are not setting a
>>> dependency for an index created inside an extension.
>>
>> Surely the index has a dependency on a table, which depends on the
>> extension?
>>
>> If you mean that you want an extension to create an index on a table
>> that doesn't belong to it, but it's assuming pre-exists, I think
>> that's just stupid and we need not support it.
>
> I don't see why that would be stupid (and I'm not sure it's up to us to just
> decide it's stupid).

Like Tomas, I am not really getting this opposition..

> I see the current behavior is documented, and I do understand why global
> objects can't be part of the extension, but for indexes it seems to violate
> POLA a bit.
>
> Is there a reason why we don't want the extension/index dependencies?

I think that we could do a better effort for indexes at least, in the
same way as we do for sequences as both are referenced in pg_class. I
don't know the effort to get that done for < 9.6, but if we can do it
at least for 9.6 and 10, which is where pg_dump is a bit smarter in
the way it deals with dependencies, we should do it.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Missing checks when malloc returns NULL...

[Michael Paquier]

On Sat, Aug 27, 2016 at 10:24 PM, Michael Paquier
 wrote:
> ./src/backend/postmaster/postmaster.c:  userDoption =
> strdup(optarg);
> [...]
> ./src/backend/bootstrap/bootstrap.c:userDoption =
> strdup(optarg);
> [...]
> ./src/backend/tcop/postgres.c:  userDoption = strdup(optarg);
> We cannot use pstrdup here because memory contexts are not set up
> here, still it would be better than crashing, but I am not sure if
> that's worth complicating this code.. Other opinions are welcome.

Those are not changed. We could have some NULL-checks but I am not
sure that's worth the complication here.

> ./contrib/vacuumlo/vacuumlo.c:  param.pg_user = strdup(optarg);
> [...]
> ./contrib/pg_standby/pg_standby.c:  triggerPath = strdup(optarg);
> [...]
> ./src/bin/pg_archivecleanup/pg_archivecleanup.c:
> additional_ext = strdup(optarg);/* Extension to remove
> Right we can do something here with pstrdup().

Those are updated with pg_strdup().

> ./contrib/spi/refint.c: plan->splan = (SPIPlanPtr *)
> malloc(sizeof(SPIPlanPtr));
> Regarding refint.c, you can see upthread. Instead of reworking the
> code it would be better to just drop it from the tree.

I'd rather see this nuked out of the surface of the code tree.

> ./src/backend/utils/adt/pg_locale.c:grouping = strdup(extlconv->grouping);
> Here that would be a failure with an elog() as this is getting used by
> things like NUM_TOCHAR_finish and PGLC_localeconv.

Done.

> ./src/pl/tcl/pltcl.c:   prodesc->user_proname =
> strdup(NameStr(procStruct->proname));
> ./src/pl/tcl/pltcl.c:   prodesc->internal_proname =
> strdup(internal_proname);
> ./src/pl/tcl/pltcl.c-   if (prodesc->user_proname == NULL ||
> prodesc->internal_proname == NULL)
> ./src/pl/tcl/pltcl.c-   ereport(ERROR,
> ./src/pl/tcl/pltcl.c-   (errcode(ERRCODE_OUT_OF_MEMORY),
> ./src/pl/tcl/pltcl.c-errmsg("out of memory")));
> Ah, yes. Here we need to do a free(prodesc) first.

Done.

> ./src/common/exec.c:putenv(strdup(env_path));
> set_pglocale_pgservice() is used at the beginning of each process run,
> meaning that a failure here would be printf(stderr), followed by
> exit() for frontend, even ECPG as this compiles with -DFRONTEND.
> Backend can use elog(ERROR) btw.

Done.

Regarding the handling of strdup in libpq the code is careful in its
handling after a review, so we had better do nothing.

After that, there are two calls to realloc and one call to malloc that
deserve some attention, though those happen in pgc.l and I am not
exactly sure how to handle them.

As Alexander's email
(https://www.postgresql.org/message-id/20160826153358.GA29981%40e733)
has broken this thread, I am attaching to this thread the analysis
report that has been generated by Alexander previously. It was
referenced in only an URL.

Attached is an updated patch addressing those extra points.
-- 
Michael
find . -type f -iname '*.c' -exec egrep -C5 '[^a-z_](malloc|realloc|strdup)' {} 
+ | less

/(malloc|realloc|strdup)




./contrib/pg_standby/pg_standby.c-  case 't':   /* Trigger file 
*/
./contrib/pg_standby/pg_standby.c:  triggerPath = strdup(optarg);
./contrib/pg_standby/pg_standby.c-  break;

./contrib/spi/refint.c: plan->splan = (SPIPlanPtr *) 
malloc(sizeof(SPIPlanPtr));
./contrib/spi/refint.c- *(plan->splan) = pplan;
./contrib/spi/refint.c- plan->nplans = 1;

./contrib/spi/refint.c: plan->splan = (SPIPlanPtr *) malloc(nrefs * 
sizeof(SPIPlanPtr));
./contrib/spi/refint.c-
./contrib/spi/refint.c- for (r = 0; r < nrefs; r++)
./contrib/spi/refint.c- {
./contrib/spi/refint.c- relname = args2[0];
./contrib/spi/refint.c-

./contrib/spi/refint.c- if (strcmp((*eplan)[i].ident, ident) == 0)
./contrib/spi/refint.c- break;
./contrib/spi/refint.c- }
./contrib/spi/refint.c- if (i != *nplans)
./contrib/spi/refint.c- return (*eplan + i);
./contrib/spi/refint.c: *eplan = (EPlan *) realloc(*eplan, (i + 1) * 
sizeof(EPlan));
./contrib/spi/refint.c- newp = *eplan + i;
./contrib/spi/refint.c- }
./contrib/spi/refint.c- else
./contrib/spi/refint.c- {
./contrib/spi/refint.c: newp = *eplan = (EPlan *) malloc(sizeof(EPlan));
./contrib/spi/refint.c- (*nplans) = i = 0;
./contrib/spi/refint.c- }
./contrib/spi/refint.c-
./contrib/spi/refint.c- newp->ident = strdup(ident);
./contrib/spi/refint.c- newp->nplans = 0;

./contrib/spi/timetravel.c- if (i != *nplans)
./contrib/spi/timetravel.c- return (*eplan + i);
./contrib/spi/timetravel.c: *eplan = (EPlan *) realloc(*eplan, (i + 1) * 
sizeof(EPlan));
./contrib/spi/timetravel.c- newp = *eplan + i;
./contrib/spi/timetravel.c- }
./contrib/spi/timetravel.c- else
./contrib/spi/timetravel.c- {
./contrib/spi/timetravel.c: newp = *eplan = (EPlan *) malloc(sizeof(EPlan));
./contrib/spi/timetravel.c- 

Re: [HACKERS] Renaming of pg_xlog and pg_clog

[Magnus Hagander]

On Mon, Aug 29, 2016 at 2:45 AM, Jim Nasby  wrote:

> On 8/26/16 4:08 PM, Andres Freund wrote:
>
>> Splitting of ephemeral data seems to have a benefit, the rest seems more
>> like rather noisy busywork to me.
>>
>
> People accidentally blowing away pg_clog or pg_xlog is a pretty common
> occurrence, and I don't think there's all that many tools that reference
> them. I think it's well worth renaming them.
>

Pretty sure every single backup tool or script out there is referencing
pg_xlog. If it's not, then it's broken...

pg_clog is a different story of course.

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

Re: [HACKERS] pg_xlogdump fails to handle WAL file with multi-page XLP_FIRST_IS_CONTRECORD data

[Fujii Masao]

On Fri, Aug 26, 2016 at 10:03 PM, Fujii Masao  wrote:
> On Wed, Mar 23, 2016 at 7:04 PM, Pavan Deolasee
>  wrote:
>>
>>
>> On Wed, Mar 23, 2016 at 1:13 PM, Michael Paquier 
>> wrote:
>>>
>>>
>>> +   /*
>>> +* Compute targetRecOff. It should typically be greater than short
>>> +* page-header since a valid record can't , but can also be zero
>>> when
>>> +* caller has supplied a page-aligned address or when we are
>>> skipping
>>> +* multi-page continuation record. It doesn't matter though
>>> because
>>> +* ReadPageInternal() will read at least short page-header worth
>>> of
>>> +* data
>>> +*/
>>> This needs some polishing, there is an unfinished sentence here.
>>>
>>> +   targetRecOff = tmpRecPtr % XLOG_BLCKSZ;
>>> targetRecOff, pageHeaderSize and targetPagePtr could be declared
>>> inside directly the new while loop.
>>
>>
>> Thanks Michael for reviewing the patch. I've fixed these issues and new
>> version is attached.
>
> The patch looks good to me. Barring any objections,
> I'll push this and back-patch to 9.3 where pg_xlogdump was added.

Pushed. Thanks!

Regards,

-- 
Fujii Masao


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Renaming of pg_xlog and pg_clog

[Michael Paquier]

On Mon, Aug 29, 2016 at 2:36 PM, Craig Ringer
 wrote:
> I don't care if it comes as part of some greater reorg or not but I'll be
> really annoyed if scope creep lands up killing the original proposal to just
> rename these dirs. I think that a simple rename should be done first. Then
> if some greater reorg is to be done it can be done shortly after. The only
> people that'll upset are folks tracking early 10.0 dev and they'll be aware
> it's coming.

Okay, so let's do it. Attached are two patches:
- 0001 renames pg_clog to pg_trans. I have let clog.c with its current
name, as well as its structures. That's the mechanical patch, the ony
interesting part being in pg_upgrade.
- 0002 renames pg_xlog to pg_wal.
-- 
Michael
From 74ff97a9520f496e0df303e777ad93d5eca054f5 Mon Sep 17 00:00:00 2001
From: Michael Paquier 
Date: Mon, 29 Aug 2016 15:05:46 +0900
Subject: [PATCH 1/2] Rename pg_clog to pg_trans

pg_upgrade is updated to handle the transfer correctly to post-10
clusters.
---
 doc/src/sgml/backup.sgml   |  4 +-
 doc/src/sgml/catalogs.sgml |  4 +-
 doc/src/sgml/config.sgml   |  2 +-
 doc/src/sgml/func.sgml |  2 +-
 doc/src/sgml/maintenance.sgml  |  8 ++--
 doc/src/sgml/ref/pg_resetxlog.sgml |  4 +-
 doc/src/sgml/ref/pg_rewind.sgml|  2 +-
 doc/src/sgml/storage.sgml  | 10 ++---
 doc/src/sgml/wal.sgml  |  2 +-
 src/backend/access/heap/heapam.c   |  4 +-
 src/backend/access/transam/README  | 12 +++---
 src/backend/access/transam/clog.c  |  2 +-
 src/backend/access/transam/commit_ts.c |  2 +-
 src/backend/access/transam/multixact.c |  2 +-
 src/backend/access/transam/subtrans.c  |  4 +-
 src/backend/access/transam/transam.c   |  2 +-
 src/backend/access/transam/twophase.c  |  4 +-
 src/backend/access/transam/xact.c  | 18 
 src/backend/access/transam/xlog.c  |  2 +-
 src/backend/commands/vacuum.c  | 10 ++---
 src/backend/postmaster/autovacuum.c|  2 +-
 src/backend/storage/buffer/README  |  2 +-
 src/backend/storage/ipc/procarray.c|  4 +-
 src/backend/utils/time/tqual.c |  6 +--
 src/bin/initdb/initdb.c|  2 +-
 src/bin/pg_upgrade/exec.c  | 79 +-
 src/bin/pg_upgrade/pg_upgrade.c| 30 +++--
 src/include/access/slru.h  |  4 +-
 28 files changed, 127 insertions(+), 102 deletions(-)

diff --git a/doc/src/sgml/backup.sgml b/doc/src/sgml/backup.sgml
index 0f09d82..18d4bd5 100644
--- a/doc/src/sgml/backup.sgml
+++ b/doc/src/sgml/backup.sgml
@@ -382,10 +382,10 @@ tar -cf backup.tar /usr/local/pgsql/data
   directories. This will not work because the
   information contained in these files is not usable without
   the commit log files,
-  pg_clog/*, which contain the commit status of
+  pg_trans/*, which contain the commit status of
   all transactions. A table file is only usable with this
   information. Of course it is also impossible to restore only a
-  table and the associated pg_clog data
+  table and the associated pg_trans data
   because that would render all other tables in the database
   cluster useless.  So file system backups only work for complete
   backup and restoration of an entire database cluster.
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 4e09e06..783d49c 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -1847,7 +1847,7 @@
All transaction IDs before this one have been replaced with a permanent
(frozen) transaction ID in this table.  This is used to track
whether the table needs to be vacuumed in order to prevent transaction
-   ID wraparound or to allow pg_clog to be shrunk.  Zero
+   ID wraparound or to allow pg_trans to be shrunk.  Zero
(InvalidTransactionId) if the relation is not a table.
   
  
@@ -2514,7 +2514,7 @@
All transaction IDs before this one have been replaced with a permanent
(frozen) transaction ID in this database.  This is used to
track whether the database needs to be vacuumed in order to prevent
-   transaction ID wraparound or to allow pg_clog to be shrunk.
+   transaction ID wraparound or to allow pg_trans to be shrunk.
It is the minimum of the per-table
pg_class.relfrozenxid values.
   
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 7c483c6..c32da94 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -5816,7 +5816,7 @@ COPY postgres_log FROM '/full/path/to/logfile.csv' WITH csv;
 

 Vacuum also allows removal of old files from the
-pg_clog subdirectory, which is why the default
+pg_trans subdirectory, which is why the default
 is a relatively low 200 million transactions.
 This parameter can only 

[HACKERS] ecpg -? option doesn't work in windows

[Haribabu Kommi]

ecpg option --help alternative -? doesn't work in windows.
In windows, the PG provided getopt_long function is used
for reading the provided options.

The getopt_long function returns '?' for invalid characters
also but it sets optopt option to 0 in case if the character
itself is a '?'. But this works for Linux and others, whereas
for windows, optopt is not 0. Because of this reason it is
failing.

I feel, from this commit 5b88b85c on wards, it is not working.
I feel instead of fixing the getopt_long function to reset optopt
parameter to zero whenever it is returning '?', I prefer fixing
the ecpg in handling the version and help options seperate.

Patch is attached. Any one prefers the getopt_long function
fix, I can produce the patch for the same.

Regards,
Hari Babu
Fujitsu Australia


ecpg_bugfix.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] GiST penalty functions [PoC]

[Andrew Borodin]

Hi, hackers!


Some time ago I put a patch to commitfest that optimizes slightly GiST
inserts and updates. It's effect in general is quite modest (15% perf
improved), but for sorted data inserts are 3 times quicker. This
effect I attribute to mitigation of deficiencies of old split
algorithm.
Alexander Korotkov advised me to lookup some features of the RR*-tree.

The RR*-tree differs from combination of Gist + cube extension in two
algorithms:
1.Algorithm to choose subtree for insertion
2.Split algorithm

This is the message regarding implementation of first one in GiST. I
think that both of this algorithms will improve querying performance.

Long story short, there are two problems in choose subtree algorithm.
1.GiST chooses subtree via penalty calculation for each entry,
while RR*-tree employs complicated conditional logic: when there are
MBBs (Minimum Bounding Box) which do not require extensions, smallest
of them is chosen; in some cases, entries are chosen not by volume,
but by theirs margin.
2.RR*-tree algorithm jumps through entry comparation non-linearly,
I think this means that GiST penalty computation function will have to
access other entries on a page.

In this message I address only first problem. I want to construct a
penalty function that will choose:
1.Entry with a zero volume and smallest margin, that can
accommodate insertion tuple without extension, if one exists;
2.Entry with smallest volume, that can accommodate insertion tuple
without extension, if one exists;
3.Entry with zero volume increase after insert and smallest margin
increase, if one exists;
4.Entry with minimal volume increase after insert.

Current cube behavior inspects only 4th option by returning as a
penalty (float) MBB’s volume increase. To implement all 4 options, I
use a hack: order of positive floats corresponds exactly to order of
integers with same binary representation (I’m not sure this stands for
every single supported platform). So I use two bits of float as if it
were integer, and all others are used as shifted bits of corresponding
float: option 4 – volume increase, 3 - margin increase, 2 – MBB
volume, 1 – MBB margin. You can check the reinterpretation cast
function pack_float() in the patch.

I’ve tested patch performance with attached test. On my machine patch
slows index construction time from 60 to 76 seconds, reduces size of
the index from 85Mb to 82Mb, reduces time of aggregates computation
from 36 seconds to 29 seconds.

I do not know: should I continue this work in cube, or it’d be better
to fork cube?
Maybe anyone have tried already RR*-tree implementation? Science
papers show very good search performance increase.


Please let me know if you have any ideas, information or interest in this topic.


Best regards, Andrey Borodin, Octonica & Ural Federal University.
diff --git a/contrib/cube/cube.c b/contrib/cube/cube.c
index 3feddef..9c8c63d 100644
--- a/contrib/cube/cube.c
+++ b/contrib/cube/cube.c
@@ -96,6 +96,7 @@ bool  cube_contains_v0(NDBOX *a, NDBOX *b);
 bool   cube_overlap_v0(NDBOX *a, NDBOX *b);
 NDBOX *cube_union_v0(NDBOX *a, NDBOX *b);
 void   rt_cube_size(NDBOX *a, double *sz);
+void   rt_cube_edge(NDBOX *a, double *sz);
 NDBOX *g_cube_binary_union(NDBOX *r1, NDBOX *r2, int *sizep);
 bool   g_cube_leaf_consistent(NDBOX *key, NDBOX *query, StrategyNumber 
strategy);
 bool   g_cube_internal_consistent(NDBOX *key, NDBOX *query, 
StrategyNumber strategy);
@@ -420,6 +421,13 @@ g_cube_decompress(PG_FUNCTION_ARGS)
PG_RETURN_POINTER(entry);
 }
 
+float pack_float(float actualValue, int realm)
+{
+   // two bits for realm, other for value
+   int realmAjustment = *((int*))/4;
+   int realCode = realm * (INT32_MAX/4) + realmAjustment; // we have 4 
realms
+   return *((float*));
+}
 
 /*
 ** The GiST Penalty method for boxes
@@ -428,6 +436,11 @@ g_cube_decompress(PG_FUNCTION_ARGS)
 Datum
 g_cube_penalty(PG_FUNCTION_ARGS)
 {
+   // REALM 0: No extension is required, volume is zerom return edge
+   // REALM 1: No extension is required, return nonzero volume
+   // REALM 2: Volume extension is zero, return nonzero edge extension
+   // REALM 3: Volume extension is nonzero, return it
+
GISTENTRY  *origentry = (GISTENTRY *) PG_GETARG_POINTER(0);
GISTENTRY  *newentry = (GISTENTRY *) PG_GETARG_POINTER(1);
float  *result = (float *) PG_GETARG_POINTER(2);
@@ -440,6 +453,32 @@ g_cube_penalty(PG_FUNCTION_ARGS)
rt_cube_size(ud, );
rt_cube_size(DatumGetNDBOX(origentry->key), );
*result = (float) (tmp1 - tmp2);
+   if(*result == 0)
+   {
+   double tmp3 = tmp1;
+   rt_cube_edge(ud, );
+   rt_cube_edge(DatumGetNDBOX(origentry->key), );
+   *result = (float) (tmp1 - tmp2);
+   if(*result == 0)
+   {
+   if(tmp3!=0)
+   

Re: [HACKERS] [PATCH] Transaction traceability - txid_status(bigint)

[Robert Haas]

On Sun, Aug 28, 2016 at 11:25 PM, Craig Ringer  wrote:
> What I'd really like is to be able to ask transam.c to handle the
> xid_in_recent_past logic, treating an attempt to read an xid from
> beyond the clog truncation threshold as a soft error indicating
> unknown xact state. But that involves delving into slru.c, and I
> really, really don't want to touch that for what should be a simple
> and pretty noninvasive utility function.

I think you're going to have to bite the bullet and do that, though, because ...

> A PG_TRY to trap ERRCODE_UNDEFINED_FILE seems like it'd be sufficient,

...I don't think this has any chance of being acceptable.  You can't
catch errors and not re-throw them.  That's bad news.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers