date:20130118


On 18.01.2013 02:35, Andres Freund wrote:

On 2013-01-18 08:24:31 +0900, Michael Paquier wrote:

On Fri, Jan 18, 2013 at 3:05 AM, Fujii Masaomasao.fu...@gmail.com  wrote:


  I encountered the problem that the timeline switch is not performed
expectedly.
I set up one master, one standby and one cascade standby. All the servers
share the archive directory. restore_command is specified in the
recovery.conf
in those two standbys.

I shut down the master, and then promoted the standby. In this case, the
cascade standby should switch to new timeline and replication should be
successfully restarted. But the timeline was never changed, and the
following
log messages were kept outputting.

sby2 LOG:  restarted WAL streaming at 0/300 on timeline 1
sby2 LOG:  replication terminated by primary server
sby2 DETAIL:  End of WAL reached on timeline 1
sby2 LOG:  restarted WAL streaming at 0/300 on timeline 1
sby2 LOG:  replication terminated by primary server
sby2 DETAIL:  End of WAL reached on timeline 1
sby2 LOG:  restarted WAL streaming at 0/300 on timeline 1
sby2 LOG:  replication terminated by primary server
sby2 DETAIL:  End of WAL reached on timeline 1


I am seeing similar issues with master at 88228e6.
This is easily reproducible by setting up 2 slaves under a master, then
kill the master. Promote slave 1 and  reconnect slave 2 to slave 1, then
you will notice that the timeline jump is not done.

I don't know if Masao tried to put in sync the slave that reconnects to the
promoted slave, but in this case slave2 stucks in potential state. That
is due to timeline that has not changed on slave2 but better to let you
know...


Ok, I know whats causing this now. Rather ugly.

Whenever accessing a page in a segment we haven't accessed before we
read the first page to do an extra bit of validation as the first page
in a segment contains more information.

Suppose timeline 1 ends at 0/6087088, xlog.c notices that WAL ends
there, wants to read the new timeline, requests record
0/06087088. xlogreader wants to do its validation and goes back to the
first page in the segment which triggers xlog.c to rerequest timeline1
to be transferred..


Hmm, so it's the same issue I thought I fixed yesterday. My patch only 
fixed it for the case that the timeline switch is in the first page of 
the segment. When it's not, you still get two calls for a WAL record, 
first one for the first page in the segment, to verify that, and then 
the page that actually contains the record. The first call leads 
XLogPageRead to think it needs to read from the old timeline.


We didn't have this problem before the xlogreader refactoring because 
XLogPageRead() was always called with the RecPtr of the record, even 
when we actually read the segment header from the file first. We'll have 
to somehow get that same information, the RecPtr of the record we're 
actually interested in, to XLogPageRead(). We could add a new argument 
to the callback for that, or we could keep xlogreader.c as it is and 
pass it through from ReadRecord to XLogPageRead() in the private struct.


An explicit argument to the callback is probably best. That's 
straightforward, and it might be useful for the callback to know the 
actual WAL position that xlogreader.c is interested in anyway. See attached.


- Heikki
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 90ba32e..3ac3b76 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -626,9 +626,10 @@ static int XLogFileRead(XLogSegNo segno, int emode, TimeLineID tli,
 			 int source, bool notexistOk);
 static int XLogFileReadAnyTLI(XLogSegNo segno, int emode, int source);
 static int XLogPageRead(XLogReaderState *xlogreader, XLogRecPtr targetPagePtr,
- int reqLen, char *readBuf, TimeLineID *readTLI);
+			 int reqLen, XLogRecPtr targetRecPtr, char *readBuf,
+			 TimeLineID *readTLI);
 static bool WaitForWALToBecomeAvailable(XLogRecPtr RecPtr, bool randAccess,
-			bool fetching_ckpt);
+			bool fetching_ckpt, XLogRecPtr tliRecPtr);
 static int	emode_for_corrupt_record(int emode, XLogRecPtr RecPtr);
 static void XLogFileClose(void);
 static void PreallocXlogFiles(XLogRecPtr endptr);
@@ -8832,7 +8833,7 @@ CancelBackup(void)
  */
 static int
 XLogPageRead(XLogReaderState *xlogreader, XLogRecPtr targetPagePtr, int reqLen,
-			 char *readBuf, TimeLineID *readTLI)
+			 XLogRecPtr targetRecPtr, char *readBuf, TimeLineID *readTLI)
 {
 	XLogPageReadPrivate *private =
 		(XLogPageReadPrivate *) xlogreader-private_data;
@@ -8880,7 +8881,8 @@ retry:
 		{
 			if (!WaitForWALToBecomeAvailable(targetPagePtr + reqLen,
 			 private-randAccess,
-			 private-fetching_ckpt))
+			 private-fetching_ckpt,
+			 targetRecPtr))
 goto triggered;
 		}
 		/* In archive or crash recovery. */
@@ -8980,11 +8982,19 @@ triggered:
 }
 
 /*
- * In standby mode, wait for the requested record to become available, either
+ * In standby

Re: [HACKERS] How to hack the storage component？


On 18.01.2013 11:02, wang chaoyong wrote:

Hi folks,

 Currently, I'm trying to read the storage component, since it's biased
towards the lower layer, seems more difficult to understand and debug. All
the materials I have found are the original papers on
http://db.cs.berkeley.edu, I'm wondering how much it have been changed
since Postgres. Could you please give me some suggestions? Thanks.


It has changed a lot since the Berkeley times. I doubt the original 
papers are of much use anymore in understanding modern PostgreSQL. 
You'll want to read src/backend/storage/smgr/README, although there 
isn't much there. Also make sure you read the Database Physical 
Storage section in the user manual, 
http://www.postgresql.org/docs/devel/static/storage.html.


- 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] Starting Postgres : user from same group

2013-01-18 Thread Albe Laurenz

Abhishek Sharma G wrote:
 As per the Postgres design, only the user who is owner of the Postgres's 
 Cluster Directory ( which has
 permission 700 ) can own the Postgres daemon process.
 But I have a requirement so that any other user belonging to the same group 
 can also start the
 Postgres daemon. It might not be very difficult to change the code for this 
 use case ( at least it
 seems to be ),
 But could some one please suggest me whether this is advisable or I will be 
 messing up with the design
 ( I am in particular worried about the shared memory ) .

That's not really a question for the hackers list.

I'd say you set the SETUID bit on the postgres executable and make sure
that only the right people can execute it.

Yours,
Laurenz Albe


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

[HACKERS] Passing connection string to pg_basebackup


On 18.01.2013 08:50, Amit Kapila wrote:

I think currently user has no way to specify TCP keepalive settings from
pg_basebackup, please let me know if there is any such existing way?


I was going to say you can just use keepalives_idle=30 in the 
connection string. But there's no way to pass a connection string to 
pg_basebackup on the command line! The usual way to pass a connection 
string is to pass it as the database name, and PQconnect will expand it, 
but that doesn't work with pg_basebackup because it hardcodes the 
database name as replication. D'oh.


You could still use environment variables and a service file to do it, 
but it's certainly more cumbersome. It clearly should be possible to 
pass a full connection string to pg_basebackup, that's an obvious oversight.


- 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] 9.3 Pre-proposal: Range Merge Join

2013-01-18 Thread Stefan Keller

Hi Jeff

2013/1/18 Jeff Davis pg...@j-davis.com:
 On Thu, 2013-01-17 at 21:03 +0100, Stefan Keller wrote:
 Hi Jeff

 I'm perhaps really late in this discussion but I just was made aware
 of that via the tweet from Josh Berkus about PostgreSQL 9.3: Current
 Feature Status

 What is the reason to digg into spatial-joins when there is PostGIS
 being a bullet-proof and fast implementation?


 Hi Stefan,

 You are certainly not too late.

 PostGIS uses the existing postgres infrastructure to do spatial joins.
 That mean it either does a cartesian product and filters the results, or
 it uses a nested loop with an inner index scan.

 That isn't too bad, but it could be better. I am trying to introduce a
 new way to do spatial joins which will perform better in more
 circumstances. For instance, we can't use an inner index if the input
 tables are actually subqueries, because we can't index a subquery.

 Regards,
 Jeff Davis

Sounds good.
Did you already had contact e.g. with Paul (cc'ed just in case)?
And will this clever index also be available within all these hundreds
of PostGIS functions?

Regards, Stefan


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

[HACKERS] Re: [HACKERS] How to hack the storage component？

2013-01-18 Thread wang chaoyong

Thank you very much.
Based on your information, I find an interesting page including your
comment:
http://code.metager.de/source/history/postgresql/src/backend/storage/smgr/README
.
Seems all guys are in the core team, really nice job.
Thanks again for your kind reply and 2 phase commit contribution.

Regards.
Chaoyong Wang




On Fri, Jan 18, 2013 at 5:26 PM, Heikki Linnakangas hlinnakan...@vmware.com
 wrote:

 On 18.01.2013 11:02, wang chaoyong wrote:

 Hi folks,

  Currently, I'm trying to read the storage component, since it's
 biased
 towards the lower layer, seems more difficult to understand and debug. All
 the materials I have found are the original papers on
 http://db.cs.berkeley.edu, I'm wondering how much it have been changed
 since Postgres. Could you please give me some suggestions? Thanks.


 It has changed a lot since the Berkeley times. I doubt the original papers
 are of much use anymore in understanding modern PostgreSQL. You'll want to
 read src/backend/storage/smgr/**README, although there isn't much there.
 Also make sure you read the Database Physical Storage section in the user
 manual, 
 http://www.postgresql.org/**docs/devel/static/storage.htmlhttp://www.postgresql.org/docs/devel/static/storage.html
 **.

 - Heikki

Re: [HACKERS] Patch for removng unused targets

2013-01-18 Thread Craig Ringer

On 12/05/2012 04:15 AM, Alexander Korotkov wrote:
 On Tue, Dec 4, 2012 at 11:52 PM, Tom Lane t...@sss.pgh.pa.us
 mailto:t...@sss.pgh.pa.us wrote:

 Alexander Korotkov aekorot...@gmail.com
 mailto:aekorot...@gmail.com writes:
  On Mon, Dec 3, 2012 at 8:31 PM, Tom Lane t...@sss.pgh.pa.us
 mailto:t...@sss.pgh.pa.us wrote:
  But having said that, I'm wondering (without having read the patch)
  why you need anything more than the existing resjunk field.

  Actually, I don't know all the cases when resjunk flag is set.
 Is it
  reliable to decide target to be used only for ORDER BY if it's
 resjunk
  and neither system or used in grouping? If it's so or there are
 some other
  cases which are easy to determine then I'll remove
 resorderbyonly flag.

 resjunk means that the target is not supposed to be output by the
 query.
 Since it's there at all, it's presumably referenced by ORDER BY or
 GROUP
 BY or DISTINCT ON, but the meaning of the flag doesn't depend on that.

 What you would need to do is verify that the target is resjunk and not
 used in any clause besides ORDER BY.  I have not read your patch, but
 I rather imagine that what you've got now is that the parser
 checks this
 and sets the new flag for consumption far downstream.  Why not
 just make
 the same check in the planner?

 A more invasive, but possibly cleaner in the long run, approach is to
 strip all resjunk targets from the query's tlist at the start of
 planning and only put them back if needed.

 BTW, when I looked at this a couple years ago, it seemed like the
 major
 problem was that the planner assumes that all plans for the query
 should
 emit the same tlist, and thus that tlist eval cost isn't a
 distinguishing factor.  Breaking that assumption seemed to require
 rather significant refactoring.  I never found the time to try to
 actually do it.


 May be there is some way to not remove items from tlist, but evade
 actual calculation?

Did you make any headway on this? Is there work in a state that's likely
to be committable for 9.3, or is it perhaps best to defer this to
post-9.3 pending further work and review?

https://commitfest.postgresql.org/action/patch_view?id=980

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

Re: [HACKERS] WIP patch for hint bit i/o mitigation

2013-01-18 Thread Craig Ringer

On 12/14/2012 09:57 PM, Amit Kapila wrote:

 I need to validate the vacuum results. It's possible that this is
 solvable by tweaking xmin check inside vacuum. Assuming that's fixed,
 the question stands: do the results justify the change?  I'd argue
 'maybe' 
 We can try with change (assuming change is small) and see if the performance
 gain is good, then discuss whether it really justifies.
 I think the main reason for Vacuum performance hit is that in the test pages
 are getting dirty only due to setting of hint bit
 by Vacuum. 

 -- I'd like to see the bulk insert performance hit reduced if
 possible.
 I think if we can improve performance for bulk-insert case, then this patch
 has much more value. 
Has there been any movement in this - more benchmarks and data showing
that it really does improve performance, or that it really isn't helpful?

-- 
 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] Passing connection string to pg_basebackup

2013-01-18 Thread Amit Kapila

On Friday, January 18, 2013 3:46 PM Heikki Linnakangas wrote:
 On 18.01.2013 08:50, Amit Kapila wrote:
  I think currently user has no way to specify TCP keepalive settings
 from
  pg_basebackup, please let me know if there is any such existing way?
 
 I was going to say you can just use keepalives_idle=30 in the
 connection string. But there's no way to pass a connection string to
 pg_basebackup on the command line! The usual way to pass a connection
 string is to pass it as the database name, and PQconnect will expand
 it,
 but that doesn't work with pg_basebackup because it hardcodes the
 database name as replication. D'oh.
 
 You could still use environment variables and a service file to do it,
 but it's certainly more cumbersome. It clearly should be possible to
 pass a full connection string to pg_basebackup, that's an obvious
 oversight.

So to solve this problem below can be done:
1. Support connection string in pg_basebackup and mention keepalives or
connection_timeout 
2. Support recv_timeout separately to provide a way to users who are not
comfortable tcp keepalives

a. 1 can be done alone 
b. 2 can be done alone
c. both 1 and 2.

With Regards,
Amit Kapila.



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

[HACKERS] More subtle issues with cascading replication over timeline switches

When a standby starts up, and catches up with the master through the 
archive, it copies the target timeline's history file from the archive 
to pg_xlog. That's enough for that standby's purposes, but if there is a 
cascading standby or pg_receivexlog connected to it, it will request the 
timeline history files of *all* timelines between the starting point and 
current target.


For example, create a master, and take a base backup from it. Use the 
base backup to initialize two standby servers. Now perform failover 
first to the first standby, and once the second standby has caught up, 
fail over again, to the second standby. (Or as a shortcut, forget about 
the standbys, and just create a recovery.conf file in the master with 
restore_command='/bin/false' and restart it. That causes a timeline 
switch. Repeat twice)


Now use the base backup to initialize a new standby server (you can kill 
and delete the old ones), using the WAL archive. Set up a second, 
cascading, standby server that connects to the first standby using 
streaming replication, not WAL archive. This cascading standby will fail 
to cross the timeline switch, because it doesn't find all the history 
files in the standby:


C 2013-01-18 13:38:46.047 EET 7695 FATAL:  could not receive timeline 
history file from the primary server: ERROR:  could not open file 
pg_xlog/0002.history: No such file or directory


Indeed, looking at the pg_xlog, it's not there (I did a couple of extra 
timeline switches:


~/pgsql.master$ ls -l data-master/pg_xlog/
total 131084
-rw--- 1 heikki heikki 16777216 Jan 18 13:38 00010001
-rw--- 1 heikki heikki 16777216 Jan 18 13:38 00010002
-rw--- 1 heikki heikki 16777216 Jan 18 13:38 00010003
-rw--- 1 heikki heikki   41 Jan 18 13:38 0002.history
-rw--- 1 heikki heikki 16777216 Jan 18 13:38 00020003
-rw--- 1 heikki heikki 16777216 Jan 18 13:38 00020004
-rw--- 1 heikki heikki 16777216 Jan 18 13:38 00020005
-rw--- 1 heikki heikki   83 Jan 18 13:38 0003.history
-rw--- 1 heikki heikki 16777216 Jan 18 13:38 00030005
-rw--- 1 heikki heikki 16777216 Jan 18 13:38 00030006
drwx-- 2 heikki heikki 4096 Jan 18 13:38 archive_status
~/pgsql.master$ ls -l data-standbyB/pg_xlog/
total 81928
-rw--- 1 heikki heikki 16777216 Jan 18 13:38 00010001
-rw--- 1 heikki heikki 16777216 Jan 18 13:38 00010002
-rw--- 1 heikki heikki 16777216 Jan 18 13:38 00020003
-rw--- 1 heikki heikki 16777216 Jan 18 13:38 00020004
-rw--- 1 heikki heikki   83 Jan 18 13:38 0003.history
-rw--- 1 heikki heikki 16777216 Jan 18 13:38 00030005
drwx-- 2 heikki heikki 4096 Jan 18 13:38 archive_status

This can be thought of as another variant of the same issue that was 
fixed by commit 60df192aea0e6458f20301546e11f7673c102101. When standby B 
scans for the latest timeline, it finds it to be 3, and it reads the 
timeline history file for 3. After that patch, it also saves it in 
pg_xlog. It doesn't save the timeline history file for timeline 2, 
because that's included in the history of timeline 3. However, when 
standby C connects, it will try to fetch all the history files that it 
doesn't have, including 0002.history, which throws the error.


A related problem is that at the segment containing the timeline switch, 
standby has only restored from archive the WAL file of the new timeline, 
not the old one. For example above, the timeline switch 1 - 2 happened 
while inserting to segment 00010003, and a copy of that 
partial segment was created with the timeline's ID as 
00020003. The standby only has the segment from the new 
timeline in pg_xlog, which is enough for that standby's purposes, but 
will cause an error when the cascading standby tries to stream it:


C 2013-01-18 13:46:12.334 EET 8579 FATAL:  error reading result of 
streaming command: ERROR:  requested WAL segment 
00010003 has already been removed


A straightforward fix would be for the standby to restore those files 
that the cascading standby needs from the WAL archive, even if they're 
not strictly required for that standby itself. But actually, isn't it a 
bad idea that we store the partial segment, 00010003 in 
this case, in the WAL archive? There's no way to tell that it's partial, 
and it can clash with a complete segment if more WAL is generated on 
that timeline. I just argued that pg_receivexlog should not do that, and 
hence keep the .partial suffix in the same situation, in 
http://www.postgresql.org/message-id/50f56245.8050...@vmware.com.


This needs some more thought. I'll try to come up with something next 
week, but if anyone has any ideas..


- Heikki


--
Sent via pgsql-hackers mailing list

Re: [HACKERS] Passing connection string to pg_basebackup


On 18.01.2013 13:41, Amit Kapila wrote:

On Friday, January 18, 2013 3:46 PM Heikki Linnakangas wrote:

On 18.01.2013 08:50, Amit Kapila wrote:

I think currently user has no way to specify TCP keepalive settings

from

pg_basebackup, please let me know if there is any such existing way?


I was going to say you can just use keepalives_idle=30 in the
connection string. But there's no way to pass a connection string to
pg_basebackup on the command line! The usual way to pass a connection
string is to pass it as the database name, and PQconnect will expand
it,
but that doesn't work with pg_basebackup because it hardcodes the
database name as replication. D'oh.

You could still use environment variables and a service file to do it,
but it's certainly more cumbersome. It clearly should be possible to
pass a full connection string to pg_basebackup, that's an obvious
oversight.


So to solve this problem below can be done:
1. Support connection string in pg_basebackup and mention keepalives or
connection_timeout
2. Support recv_timeout separately to provide a way to users who are not
comfortable tcp keepalives

a. 1 can be done alone
b. 2 can be done alone
c. both 1 and 2.


Right. Let's do just 1 for now. An general application level, non-TCP, 
keepalive message at the libpq level might be a good idea, but that's a 
much larger patch, definitely not 9.3 material.


- 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] Teaching pg_receivexlog to follow timeline switches


On 18.01.2013 06:38, Phil Sorber wrote:

On Tue, Jan 15, 2013 at 9:05 AM, Heikki Linnakangas
hlinnakan...@vmware.com  wrote:

Now that a standby server can follow timeline switches through streaming
replication, we should do teach pg_receivexlog to do the same. Patch
attached.


Is it possible to re-use walreceiver code from the backend?

I was thinking that it would actually be very useful to have the whole
replication functionality modularized and in a standalone binary that
could act as a replication proxy and WAL archiver that could run
without all the overhead of an entire PG instance


There's much sense in trying to extract that into a stand-along module. 
src/bin/pg_basebackup/receivelog.c is about 1000 lines of code at the 
moment, and it looks quite different from the corresponding code in the 
backend, because it doesn't have all the backend infrastructure available.


- 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] could not create directory ...: File exists

Tom,

* Tom Lane (t...@sss.pgh.pa.us) wrote:
 Don't see what.  The main reason we've not yet attempted a global fix is
 that the most straightforward way (take a new snapshot each time we
 start a new SnapshotNow scan) seems too expensive.  But CREATE DATABASE
 is so expensive that the cost of an extra snapshot there ain't gonna
 matter.

  Patch attached.  Passes the regression tests and our internal testing
  shows that it eliminates the error we were seeing (and doesn't cause
  blocking, which is even better).

  We have a workaround in place for our build system (more-or-less
  don't do that approach), but it'd really be great if this was
  back-patched and in the next round of point releases.  Glancing
  through the branches, looks like it should apply pretty cleanly.

Thanks,

Stephen
colordiff --git a/src/backend/commands/dbcommands.c b/src/backend/commands/dbcommands.c
new file mode 100644
index 1f6e02d..94e1a5d
*** a/src/backend/commands/dbcommands.c
--- b/src/backend/commands/dbcommands.c
*** createdb(const CreatedbStmt *stmt)
*** 131,136 
--- 131,137 
  	int			notherbackends;
  	int			npreparedxacts;
  	createdb_failure_params fparms;
+ 	Snapshot	snapshot;
  
  	/* Extract options from the statement node tree */
  	foreach(option, stmt-options)
*** createdb(const CreatedbStmt *stmt)
*** 535,540 
--- 536,555 
  	RequestCheckpoint(CHECKPOINT_IMMEDIATE | CHECKPOINT_FORCE | CHECKPOINT_WAIT);
  
  	/*
+ 	 * Take a snapshot to use while scanning through pg_tablespace.  As we
+ 	 * create directories and copy files around, this process could take a
+ 	 * while, so also register that snapshot.
+ 	 *
+ 	 * Traversing pg_tablespace with an MVCC snapshot is necessary to provide
+ 	 * us with a consistent view of the tablespaces that exist.  Using
+ 	 * SnapshotNow here would risk seeing the same tablespace multiple times
+ 	 * or, worse, not seeing a tablespace at all if its tuple is moved around
+ 	 * by other concurrent operations (eg: due to the ACL on the tablespace
+ 	 * being updated).
+ 	 */
+ 	snapshot = RegisterSnapshot(GetTransactionSnapshot());
+ 
+ 	/*
  	 * Once we start copying subdirectories, we need to be able to clean 'em
  	 * up if we fail.  Use an ENSURE block to make sure this happens.  (This
  	 * is not a 100% solution, because of the possibility of failure during
*** createdb(const CreatedbStmt *stmt)
*** 551,557 
  		 * each one to the new database.
  		 */
  		rel = heap_open(TableSpaceRelationId, AccessShareLock);
! 		scan = heap_beginscan(rel, SnapshotNow, 0, NULL);
  		while ((tuple = heap_getnext(scan, ForwardScanDirection)) != NULL)
  		{
  			Oid			srctablespace = HeapTupleGetOid(tuple);
--- 566,572 
  		 * each one to the new database.
  		 */
  		rel = heap_open(TableSpaceRelationId, AccessShareLock);
! 		scan = heap_beginscan(rel, snapshot, 0, NULL);
  		while ((tuple = heap_getnext(scan, ForwardScanDirection)) != NULL)
  		{
  			Oid			srctablespace = HeapTupleGetOid(tuple);
*** createdb(const CreatedbStmt *stmt)
*** 656,661 
--- 671,679 
  	PG_END_ENSURE_ERROR_CLEANUP(createdb_failure_callback,
  PointerGetDatum(fparms));
  
+ 	/* Free our snapshot */
+ 	UnregisterSnapshot(snapshot);
+ 
  	return dboid;
  }
  


signature.asc
Description: Digital signature

Re: [HACKERS] Review: Patch to compute Max LSN of Data Pages

2013-01-18 Thread Amit kapila

Please find the rebased Patch for Compute MAX LSN.



There was one compilation error as undefined reference to XLByteLT  as 
earlier  XLogRecPtr was a structure as
typedef struct XLogRecPtr
{
uint32xlogid;   
 /* log file #, 0 based */
uint32xrecoff;
/* byte offset of location in log file */
} XLogRecPtr;
So in order to compare two LSN, there was one macro as XLByteLT to compare both 
fields.
But now XLogRecPtr  has been changed as just uint64 and so XLByteLT is removed.
So the change done is to replace XLByteLT(*maxlsn, pagelsn) with (*maxlsn  
pagelsn).

Muhammad, Can you verify if every thing is okay, then this can be marked as 
Ready for Committer



With Regards,

Amit Kapila.


pg_computemaxlsn_v5.patch
Description: pg_computemaxlsn_v5.patch

-- 
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] Passing connection string to pg_basebackup

2013-01-18 Thread Amit Kapila

On Friday, January 18, 2013 5:35 PM Heikki Linnakangas wrote:
 On 18.01.2013 13:41, Amit Kapila wrote:
  On Friday, January 18, 2013 3:46 PM Heikki Linnakangas wrote:
  On 18.01.2013 08:50, Amit Kapila wrote:
  I think currently user has no way to specify TCP keepalive settings
  from
  pg_basebackup, please let me know if there is any such existing
 way?
 
  I was going to say you can just use keepalives_idle=30 in the
  connection string. But there's no way to pass a connection string to
  pg_basebackup on the command line! The usual way to pass a
 connection
  string is to pass it as the database name, and PQconnect will expand
  it,
  but that doesn't work with pg_basebackup because it hardcodes the
  database name as replication. D'oh.
 
  You could still use environment variables and a service file to do
 it,
  but it's certainly more cumbersome. It clearly should be possible to
  pass a full connection string to pg_basebackup, that's an obvious
  oversight.
 
  So to solve this problem below can be done:
  1. Support connection string in pg_basebackup and mention keepalives
 or
  connection_timeout
  2. Support recv_timeout separately to provide a way to users who are
 not
  comfortable tcp keepalives
 
  a. 1 can be done alone
  b. 2 can be done alone
  c. both 1 and 2.
 
 Right. Let's do just 1 for now. 

I shall fix it as Review comment and update the patch and change the
location from CF-3 to current CF.

 An general application level, non-TCP,
 keepalive message at the libpq level might be a good idea, but that's a
 much larger patch, definitely not 9.3 material.


With Regards,
Amit Kapila.



-- 
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] Passing connection string to pg_basebackup

2013-01-18 Thread Dimitri Fontaine

Heikki Linnakangas hlinnakan...@vmware.com writes:
 You could still use environment variables and a service file to do it, but
 it's certainly more cumbersome. It clearly should be possible to pass a full
 connection string to pg_basebackup, that's an obvious oversight.

FWIW, +1. I would consider it a bugfix (backpatch, etc).

Regards,
-- 
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support


-- 
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] Materialized views WIP patch

2013-01-18 Thread Thom Brown

On 17 January 2013 16:03, Thom Brown t...@linux.com wrote:

 On 16 January 2013 17:25, Thom Brown t...@linux.com wrote:

 On 16 January 2013 17:20, Kevin Grittner kgri...@mail.com wrote:

 Thom Brown wrote:

  Some weirdness:
 
  postgres=# CREATE VIEW v_test2 AS SELECT 1 moo;
  CREATE VIEW
  postgres=# CREATE MATERIALIZED VIEW mv_test2 AS SELECT moo, 2*moo FROM
  v_test2 UNION ALL SELECT moo, 3*moo FROM v_test2;
  SELECT 2
  postgres=# \d+ mv_test2
   Materialized view public.mv_test2
   Column | Type | Modifiers | Storage | Stats target | Description
  --+-+---+-+--+-
   moo | integer | | plain | |
   ?column? | integer | | plain | |
  View definition:
   SELECT *SELECT* 1.moo, *SELECT* 1.?column?;

 You are very good at coming up with these, Thom!

 Will investigate.

 Can you confirm that *selecting* from the MV works as you would
 expect; it is just the presentation in \d+ that's a problem?


 Yes, nothing wrong with using the MV, or refreshing it:

 postgres=# TABLE mv_test2;
  moo | ?column?
 -+--
1 |2
1 |3
 (2 rows)

 postgres=# SELECT * FROM mv_test2;
  moo | ?column?
 -+--
1 |2
1 |3
 (2 rows)

 postgres=# REFRESH MATERIALIZED VIEW mv_test2;
 REFRESH MATERIALIZED VIEW

 But a pg_dump of the MV has the same issue as the view definition:

 --
 -- Name: mv_test2; Type: MATERIALIZED VIEW; Schema: public; Owner: thom;
 Tablespace:
 --

 CREATE MATERIALIZED VIEW mv_test2 (
 moo,
 ?column?
 ) AS
 SELECT *SELECT* 1.moo, *SELECT* 1.?column?
   WITH NO DATA;


 A separate issue is with psql tab-completion:

 postgres=# COMMENT ON MATERIALIZED VIEW ^IIS

 This should be offering MV names instead of prematurely providing the IS
 keyword.


Also in doc/src/sgml/ref/alter_materialized_view.sgml:

s/materailized/materialized/


In src/backend/executor/execMain.c:

s/referrenced/referenced/

-- 
Thom

Re: [HACKERS] Event Triggers: adding information

On 2013-01-17 22:39:18 -0500, Robert Haas wrote:
 On Thu, Jan 17, 2013 at 8:33 PM, Andres Freund and...@2ndquadrant.com wrote:
  I have no problem requiring C code to use the even data, be it via hooks
  or via C functions called from event triggers. The problem I have with
  putting in some hooks is that I doubt that you can find sensible spots
  with enough information to actually recreate the DDL for a remote system
  without doing most of the work for command triggers.
 
 It should be noted that the point of KaiGai's work over the last three
 years has been to solve exactly this problem.  Well, KaiGai wants to
 check security rather than do replication, but he wants to be able
 grovel through the entire node tree and make security decisions based
 on stuff core PG doesn't care about, so in effect the requirements are
 identical.  Calling the facility event triggers rather than object
 access hooks doesn't make the underlying problem any easier to solve.
  I actually believe that the object access hook stuff is getting
 pretty close to a usable solution if you don't mind coding in C, but
 I've had trouble convincing anyone else of that.
 
 I find it a shame that it hasn't been taken more seriously, because it
 really does solve the same problem.  sepgsql, for example, has no
 trouble at all checking permissions for dropped objects.  You can't
 call procedural code from the spot where we've got that hook, but you
 sure can call C code, with the usual contract that if it breaks you
 get to keep both pieces.  The CREATE stuff works fine too.  Support
 for ALTER is not all there yet, but that's because it's a hard
 problem.

I don't have a problem reusing the object access infrastructure at all. I just
don't think its providing even remotely enough. You have (co-)written that
stuff, so you probably know more than I do, but could you explain to me how it
could be reused to replicate a CREATE TABLE?

Problems I see:
- afaics for CREATE TABLE the only hook is in ATExecAddColumn
- no access to the CreateStm, making it impossible to decipher whether e.g. a
  sequence was created as part of this or not
- No way to regenerate the table definition for execution on the remote system
  without creating libpqdump.

Greetings,

Andres Freund

-- 
 Andres Freund 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] WIP patch for hint bit i/o mitigation

2013-01-18 Thread Merlin Moncure

On Fri, Jan 18, 2013 at 5:34 AM, Craig Ringer cr...@2ndquadrant.com wrote:
 On 12/14/2012 09:57 PM, Amit Kapila wrote:

 I need to validate the vacuum results. It's possible that this is
 solvable by tweaking xmin check inside vacuum. Assuming that's fixed,
 the question stands: do the results justify the change?  I'd argue
 'maybe'
 We can try with change (assuming change is small) and see if the performance
 gain is good, then discuss whether it really justifies.
 I think the main reason for Vacuum performance hit is that in the test pages
 are getting dirty only due to setting of hint bit
 by Vacuum.

 -- I'd like to see the bulk insert performance hit reduced if
 possible.
 I think if we can improve performance for bulk-insert case, then this patch
 has much more value.
 Has there been any movement in this - more benchmarks and data showing
 that it really does improve performance, or that it really isn't helpful?

Atri is working on that.  I don't know if he's going to pull it out
though in time so we may have to pull the patch from this fest.  My
take on the current patch is that the upside case is pretty clear but
the bulk insert performance impact needs to be figured out and
mitigated (that's what Atri is working on).

merlin


-- 
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: Slave enters in recovery and promotes when WAL stream with master is cut + delay master/slave

Andres Freund escribió:
 On 2013-01-18 08:24:31 +0900, Michael Paquier wrote:
 
  The replication delays are still here.
 
 That one is caused by this nice bug, courtesy of yours truly:
 diff --git a/src/backend/access/transam/xlog.c 
 b/src/backend/access/transam/xlog.c
 index 90ba32e..1174493 100644
 --- a/src/backend/access/transam/xlog.c
 +++ b/src/backend/access/transam/xlog.c
 @@ -8874,7 +8874,7 @@ retry:
 /* See if we need to retrieve more data */
 if (readFile  0 ||
 (readSource == XLOG_FROM_STREAM 
 -receivedUpto = targetPagePtr + reqLen))
 +receivedUpto  targetPagePtr + reqLen))
 {
 if (StandbyMode)
 {

Pushed.

-- 
Álvaro Herrerahttp://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: [PATCH 1/5] Centralize Assert* macros into c.h so its common between backend/frontend

On 2013-01-08 15:55:24 -0500, Tom Lane wrote:
 Alvaro Herrera alvhe...@2ndquadrant.com writes:
  Andres Freund wrote:
  Sorry, misremembered the problem somewhat. The problem is that code that
  includes postgres.h atm ends up with ExceptionalCondition() et
  al. declared even if FRONTEND is defined. So if anything uses an assert
  you need to provide wrappers for those which seems nasty. If they are
  provided centrally and check for FRONTEND that problem doesn't exist.
 
  I think the right fix here is to fix things so that postgres.h is not
  necessary.  How hard is that?  Maybe it just requires some more
  reshuffling of xlog headers.
 
 That would definitely be the ideal fix.  In general, #include'ing
 postgres.h into code that's not backend code opens all kinds of hazards
 that are likely to bite us sooner or later; the incompatibility of the
 Assert definitions is just the tip of that iceberg.  (In the past we've
 had issues around stdio.h providing different definitions in the two
 environments, for example.)

I agree that going in that direction is a good thing, but imo that doesn't
really has any bearing on whether this patch is a good/bad idea...

 But having said that, I'm also now remembering that we have files in
 src/port/ and probably elsewhere that try to work in both environments
 by just #include'ing c.h directly (relying on the Makefile to supply
 -DFRONTEND or not).  If we wanted to support Assert use in such files,
 we would have to move at least the Assert() macro definitions into c.h
 as Andres is proposing.  Now, I've always thought that #include'ing c.h
 directly was kind of an ugly hack, but I don't have a better design than
 that to offer ATM.

Imo having some common dumping ground for things that concern both environments
is a good idea. I don't think c.h would be the best place for it when
redesigning from ground up, but were not doing that so imo its acceptable as
long as we take care not to let it grow too much.

Here's a trivially updated patch which also defines AssertArg() for
FRONTEND-ish environments since Alvaro added one in xlogreader.c.

Do we agree this is the way forward, at least for now?

Greetings,

Andres Freund

-- 
 Andres Freund 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] proposal: fix corner use case of variadic fuctions usage

Pavel,

* Pavel Stehule (pavel.steh...@gmail.com) wrote:
 Now I fixed these issues and I hope  so it will work on all platforms

As mentioned on the commitfest application, this needs documentation.
That is not the responsibility of the committer; if you need help, then
please ask for it.

I've also done a quick review of it.

The massive if() block being added to execQual.c:ExecMakeFunctionResult
really looks like it might be better pulled out into a function of its
own.

What does use element_type depends for dimensions mean and why is it
a ToDo?  When will it be done?

Overall, the comments really need to be better.  Things like this:

+   /* create short lived copies of fmgr data */
+   fmgr_info_copy(sfinfo, fcinfo-flinfo, fcinfo-flinfo-fn_mcxt);
+   memcpy(scinfo, fcinfo, sizeof(FunctionCallInfoData));
+   scinfo-flinfo = sfinfo;


Really don't cut it, imv.  *Why* are we creating a copy of the fmgr
data?  Why does it need to be short lived?  And what is going to happen
later when you do this?:

fcinfo = scinfo;
MemoryContextSwitchTo(oldContext);

Is there any reason to worry about the fact that fcache-fcinfo_data no
longer matches the fcinfo that we're working on?  What if there are
other references made to it in the future?  These memcpy's and pointer
foolishness really don't strike me as being a well thought out approach.

There are other similar comments throughout which really need to be
rewritten to address why we're doing something, not what is being done-
you can read the code for that.

Marking this Waiting on Author.

Thanks,

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] WIP patch for hint bit i/o mitigation

2013-01-18 Thread Atri Sharma



On 18-Jan-2013, at 17:04, Craig Ringer cr...@2ndquadrant.com wrote:

 On 12/14/2012 09:57 PM, Amit Kapila wrote:
 
 I need to validate the vacuum results. It's possible that this is
 solvable by tweaking xmin check inside vacuum. Assuming that's fixed,
 the question stands: do the results justify the change?  I'd argue
 'maybe'
 We can try with change (assuming change is small) and see if the performance
 gain is good, then discuss whether it really justifies.
 I think the main reason for Vacuum performance hit is that in the test pages
 are getting dirty only due to setting of hint bit
 by Vacuum. 
 
 -- I'd like to see the bulk insert performance hit reduced if
 possible.
 I think if we can improve performance for bulk-insert case, then this patch
 has much more value.
 Has there been any movement in this - more benchmarks and data showing
 that it really does improve performance, or that it really isn't helpful?
 
 -- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training  Services

Hello all,

Sorry for the delay in updating the hackers list with the current status.

I recently did some profiling using perf on PostgreSQL 9.2 with and without our 
patch.

I noticed that maximum time is being spent on heapgettup function. Further 
investigation and a bit of a hunch leads me to believe that we may be adversely 
affecting the visibility map optimisation that directly interact with the 
visibility functions, that our patch straight away affects.

If this is the case, we may really need to get down to the design of our patch, 
and maybe see which visibility function/functions we are affecting, and see if 
we can mitigate the affect.

Please let me know your inputs on this.

Regards,

Atri


 


-- 
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] Event Triggers: adding information

On Fri, Jan 18, 2013 at 9:07 AM, Andres Freund and...@2ndquadrant.com wrote:
 On 2013-01-17 22:39:18 -0500, Robert Haas wrote:
 On Thu, Jan 17, 2013 at 8:33 PM, Andres Freund and...@2ndquadrant.com 
 wrote:
  I have no problem requiring C code to use the even data, be it via hooks
  or via C functions called from event triggers. The problem I have with
  putting in some hooks is that I doubt that you can find sensible spots
  with enough information to actually recreate the DDL for a remote system
  without doing most of the work for command triggers.

 It should be noted that the point of KaiGai's work over the last three
 years has been to solve exactly this problem.  Well, KaiGai wants to
 check security rather than do replication, but he wants to be able
 grovel through the entire node tree and make security decisions based
 on stuff core PG doesn't care about, so in effect the requirements are
 identical.  Calling the facility event triggers rather than object
 access hooks doesn't make the underlying problem any easier to solve.
  I actually believe that the object access hook stuff is getting
 pretty close to a usable solution if you don't mind coding in C, but
 I've had trouble convincing anyone else of that.

 I find it a shame that it hasn't been taken more seriously, because it
 really does solve the same problem.  sepgsql, for example, has no
 trouble at all checking permissions for dropped objects.  You can't
 call procedural code from the spot where we've got that hook, but you
 sure can call C code, with the usual contract that if it breaks you
 get to keep both pieces.  The CREATE stuff works fine too.  Support
 for ALTER is not all there yet, but that's because it's a hard
 problem.

 I don't have a problem reusing the object access infrastructure at all. I just
 don't think its providing even remotely enough. You have (co-)written that
 stuff, so you probably know more than I do, but could you explain to me how it
 could be reused to replicate a CREATE TABLE?

 Problems I see:
 - afaics for CREATE TABLE the only hook is in ATExecAddColumn

No, there's one also in heap_create_with_catalog.  Took me a minute to
find it, as it does not use InvokeObjectAccessHook.  The idea is that
OAT_POST_CREATE fires once per object creation, regardless of the
object type - table, column, whatever.

 - no access to the CreateStm, making it impossible to decipher whether e.g. a
   sequence was created as part of this or not

Yep, that's a problem.  We could of course add additional hook sites
with relevant context information - that's what this infrastructure is
supposed to allow for.

 - No way to regenerate the table definition for execution on the remote system
   without creating libpqdump.

IMHO, that is one of the really ugly problems that we haven't come up
with a good solution for yet.  If you want to replicate DDL, you have
basically three choices:

1. copy over the statement text that was used on the origin server and
hope none of the corner cases bite you
2. come up with some way of reconstituting a DDL statement based on
(a) the parse tree or (b) what the server actually decided to do
3. reconstitute the state of the object from the catalogs after the
command has run

(2a) differs from (2b) for things like CREATE INDEX, where the index
name might be left for the server to determine, but when replicating
you'd like to get the same name out.  (3) is workable for CREATE but
not ALTER or DROP.

The basic problem here is that (1) and (3) are not very
reliable/complete and (2) is a lot of work and introduces a huge code
maintenance burden.  But it's unfair to pin that on the object-access
hook mechanism - any reverse-parsing or catalog-deconstruction
solution for DDL is going to have that problem.  The decision we have
to make as a community is whether we're prepared to support and
maintain that code for the indefinite future.  Although I think it's
easy to say yes, because DDL replication would be really cool - and
I sure agree with that - I think it needs to be thought through a bit
more deeply than that.

I have been involved in PostgreSQL development for about 4.5 years
now.  This is less time than many people here, but it's still long
enough to say a whole lot of people ask for some variant of this idea,
and yet I have yet to see anybody produce a complete, working version
of this functionality and maintain it outside of the PostgreSQL tree
for one release cycle (did I miss something?).  If we pull that
functionality into core, that's just a way of taking work that
nobody's been willing to do and force the responsibility to be spread
across the whole development group.  Now, sometimes it is OK to do
that, but sometimes it means that we're just bolting more things onto
the already-long list of reasons for which patches can be rejected.
Anybody who is now feeling uncomfortable at the prospect of not having
that facility in core ought to think about how they'll feel about,
say, one additional patch

Re: [HACKERS] recursive view syntax

* Peter Eisentraut (pete...@gmx.net) wrote:
 I noticed we don't implement the recursive view syntax, even though it's
 part of the standard SQL feature set for recursive queries.  Here is a
 patch to add that.  It basically converts
 
 CREATE RECURSIVE VIEW name (columns) AS SELECT ...;
 
 to
 
 CREATE VIEW name AS WITH RECURSIVE name (columns) AS (SELECT ...) SELECT
 columns FROM name;

I've done another review of this patch and it looks pretty good to me.
My only complaint is that there isn't a single comment inside
makeRecursiveViewSelect().

One other thought is- I'm guessing this isn't going to work:

CREATE RECURSIVE VIEW name (columns) AS WITH ... SELECT ...;

Does the spec explicitly allow or disallow that?  Should we provide any
comments about it?

Thanks,

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Event Triggers: adding information

2013-01-18 Thread Simon Riggs

On 18 January 2013 02:48, Tom Lane t...@sss.pgh.pa.us wrote:
 Andres Freund and...@2ndquadrant.com writes:
 I have no problem requiring C code to use the even data, be it via hooks
 or via C functions called from event triggers. The problem I have with
 putting in some hooks is that I doubt that you can find sensible spots
 with enough information to actually recreate the DDL for a remote system
 without doing most of the work for command triggers.

 most of the work?  No, I don't think so.  Here's the thing that's
 bothering me about that: if the use-case that everybody is worried about
 is replication, then triggers of any sort are the Wrong Thing, IMO.

 The difference between a replication hook and a trigger is that a
 replication hook has no business changing any state of the local system,
 whereas a trigger *has to be expected to change the state of the local
 system* because if it has no side-effects you might as well not bother
 firing it.  And the fear of having to cope with arbitrary side-effects
 occuring in the midst of DDL is about 80% of my angst with this whole
 concept.

 If we're only interested in replication, let's put in some hooks whose
 contract does not allow for side-effects on the local catalogs, and be
 done.  Otherwise we'll be putting in man-years of unnecessary (or at
 least unnecessary for this use-case) work.


I would be happy to see something called replication triggers or
replication hooks committed.

The things I want to be able to do are:
* Replicate DDL for use on other systems, in multiple ways selected by
user, possibly 1 at same time
* Put in a block to prevent all DDL, for use during upgrades or just a
general lockdown, then remove it again when complete.
* Perform auditing of DDL statements (which requires more info than
simply the text submitted)

If there are questions about wider functionality, or risks with that,
then reducing the functionality is OK, for me. The last thing we need
is a security hole introduced by this, or weird behaviour from people
interfering with things. (If you want that, write an executor plugin
and cook your own spaghetti).

We can do this by declaring clearly that there are limits on what can
be achieved with event triggers, perhaps even testing to check bad
things haven't been allowed to occur. I don't see that as translating
into massive change in the patch, just focusing on the main cases,
documenting that and perhaps introducing some restrictions on wider
usage. We will likely slowly expand the functionality of event
triggers over time, so keeping things general still works as a long
range goal.

-- 
 Simon Riggs   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

[HACKERS] Re: [PATCH] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4)

On 2013-01-09 15:07:10 -0500, Tom Lane wrote:
 I would like to know if other people get comparable results on other
 hardware (non-Intel hardware would be especially interesting).  If this
 result holds up across a range of platforms, I'll withdraw my objection
 to making palloc a plain function.

Unfortunately nobody spoke up and I don't have any non-intel hardware
anymore. Well, aside from my phone that is...

Updated patch attached, the previous version missed some contrib modules.

I like the diffstat:
 41 files changed, 224 insertions(+), 636 deletions(-)

Greetings,

Andres Freund

-- 
 Andres Freund http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training  Services
From e4960b1659c8bc33661ff07b2ba3cccef653c8fc Mon Sep 17 00:00:00 2001
From: Andres Freund and...@anarazel.de
Date: Wed, 9 Jan 2013 12:05:37 +0100
Subject: [PATCH] Provide a common malloc wrappers and palloc et al. emulation
 for frontend'ish environs

---
 contrib/oid2name/oid2name.c|  52 +
 contrib/pg_upgrade/pg_upgrade.h|   5 +-
 contrib/pg_upgrade/util.c  |  49 -
 contrib/pgbench/pgbench.c  |  54 +-
 src/backend/storage/file/copydir.c |  12 +--
 src/backend/utils/mmgr/mcxt.c  |  78 +++-
 src/bin/initdb/initdb.c|  40 +-
 src/bin/pg_basebackup/pg_basebackup.c  |   2 +-
 src/bin/pg_basebackup/pg_receivexlog.c |   1 +
 src/bin/pg_basebackup/receivelog.c |   1 +
 src/bin/pg_basebackup/streamutil.c |  38 +-
 src/bin/pg_basebackup/streamutil.h |   4 -
 src/bin/pg_ctl/pg_ctl.c|  39 +-
 src/bin/pg_dump/Makefile   |   6 +-
 src/bin/pg_dump/common.c   |   1 -
 src/bin/pg_dump/compress_io.c  |   1 -
 src/bin/pg_dump/dumpmem.c  |  76 ---
 src/bin/pg_dump/dumpmem.h  |  22 --
 src/bin/pg_dump/dumputils.c|   1 -
 src/bin/pg_dump/dumputils.h|   1 +
 src/bin/pg_dump/pg_backup_archiver.c   |   1 -
 src/bin/pg_dump/pg_backup_custom.c |   2 +-
 src/bin/pg_dump/pg_backup_db.c |   1 -
 src/bin/pg_dump/pg_backup_directory.c  |   1 -
 src/bin/pg_dump/pg_backup_null.c   |   1 -
 src/bin/pg_dump/pg_backup_tar.c|   1 -
 src/bin/pg_dump/pg_dump.c  |   1 -
 src/bin/pg_dump/pg_dump_sort.c |   1 -
 src/bin/pg_dump/pg_dumpall.c   |   1 -
 src/bin/pg_dump/pg_restore.c   |   1 -
 src/bin/pg_resetxlog/pg_resetxlog.c|   5 +-
 src/bin/psql/common.c  |  50 -
 src/bin/psql/common.h  |  10 +--
 src/bin/scripts/common.c   |  49 -
 src/bin/scripts/common.h   |   5 +-
 src/include/port/palloc.h  |  19 +
 src/include/utils/palloc.h |  12 +--
 src/port/Makefile  |   8 +-
 src/port/dirmod.c  |  75 +--
 src/port/palloc.c  | 129 +
 src/tools/msvc/Mkvcbuild.pm|   4 +-
 41 files changed, 224 insertions(+), 636 deletions(-)
 delete mode 100644 src/bin/pg_dump/dumpmem.c
 delete mode 100644 src/bin/pg_dump/dumpmem.h
 create mode 100644 src/include/port/palloc.h
 create mode 100644 src/port/palloc.c

diff --git a/contrib/oid2name/oid2name.c b/contrib/oid2name/oid2name.c
index a666731..dfd8105 100644
--- a/contrib/oid2name/oid2name.c
+++ b/contrib/oid2name/oid2name.c
@@ -9,6 +9,8 @@
  */
 #include postgres_fe.h
 
+#include port/palloc.h
+
 #include unistd.h
 #ifdef HAVE_GETOPT_H
 #include getopt.h
@@ -50,9 +52,6 @@ struct options
 /* function prototypes */
 static void help(const char *progname);
 void		get_opts(int, char **, struct options *);
-void	   *pg_malloc(size_t size);
-void	   *pg_realloc(void *ptr, size_t size);
-char	   *pg_strdup(const char *str);
 void		add_one_elt(char *eltname, eary *eary);
 char	   *get_comma_elts(eary *eary);
 PGconn	   *sql_conn(struct options *);
@@ -201,53 +200,6 @@ help(const char *progname)
 		   progname, progname);
 }
 
-void *
-pg_malloc(size_t size)
-{
-	void	   *ptr;
-
-	/* Avoid unportable behavior of malloc(0) */
-	if (size == 0)
-		size = 1;
-	ptr = malloc(size);
-	if (!ptr)
-	{
-		fprintf(stderr, out of memory\n);
-		exit(1);
-	}
-	return ptr;
-}
-
-void *
-pg_realloc(void *ptr, size_t size)
-{
-	void	   *result;
-
-	/* Avoid unportable behavior of realloc(NULL, 0) */
-	if (ptr == NULL  size == 0)
-		size = 1;
-	result = realloc(ptr, size);
-	if (!result)
-	{
-		fprintf(stderr, out of memory\n);
-		exit(1);
-	}
-	return result;
-}
-
-char *
-pg_strdup(const char *str)
-{
-	char	   *result = strdup(str);
-
-	if (!result)
-	{
-		fprintf(stderr, out of memory\n);
-		exit(1);
-	}
-	return result;
-}
-
 /*
  * add_one_elt
  *
diff --git a/contrib/pg_upgrade/pg_upgrade.h b/contrib/pg_upgrade/pg_upgrade.h
index d5c3fa9..a917b5b 100644
---

Re: [HACKERS] Re: [PATCH 1/5] Centralize Assert* macros into c.h so its common between backend/frontend

On 2013-01-18 15:48:01 +0100, Andres Freund wrote:
 Here's a trivially updated patch which also defines AssertArg() for
 FRONTEND-ish environments since Alvaro added one in xlogreader.c.

This time for real. Please.

-- 
 Andres Freund http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training  Services
From 0190dd4d9a6d8e638727eaee71cb1390e6bbe88e Mon Sep 17 00:00:00 2001
From: Andres Freund and...@anarazel.de
Date: Tue, 8 Jan 2013 17:59:10 +0100
Subject: [PATCH] Centralize Assert* macros into c.h so its common between
 backend/frontend

c.h already had parts of the assert support (StaticAssert*) and its the shared
file between postgres.h and postgres_fe.h. This makes it easier to build
frontend programs which have to do the hack.
---
 src/include/c.h   | 67 +++
 src/include/postgres.h| 54 ++
 src/include/postgres_fe.h | 12 -
 3 files changed, 69 insertions(+), 64 deletions(-)

diff --git a/src/include/c.h b/src/include/c.h
index 59af5b5..e2aefa9 100644
--- a/src/include/c.h
+++ b/src/include/c.h
@@ -694,6 +694,73 @@ typedef NameData *Name;
 
 
 /*
+ * USE_ASSERT_CHECKING, if defined, turns on all the assertions.
+ * - plai  9/5/90
+ *
+ * It should _NOT_ be defined in releases or in benchmark copies
+ */
+
+/*
+ * Assert() can be used in both frontend and backend code. In frontend code it
+ * just calls the standard assert, if it's available. If use of assertions is
+ * not configured, it does nothing.
+ */
+#ifndef USE_ASSERT_CHECKING
+
+#define Assert(condition)
+#define AssertMacro(condition)	((void)true)
+#define AssertArg(condition)
+#define AssertState(condition)
+
+#elif defined FRONTEND
+
+#include assert.h
+#define Assert(p) assert(p)
+#define AssertMacro(p)	((void) assert(p))
+#define AssertArg(condition) assert(condition)
+#define AssertState(condition) assert(condition)
+
+#else /* USE_ASSERT_CHECKING  FRONTEND */
+
+/*
+ * Trap
+ *		Generates an exception if the given condition is true.
+ */
+#define Trap(condition, errorType) \
+	do { \
+		if ((assert_enabled)  (condition)) \
+			ExceptionalCondition(CppAsString(condition), (errorType), \
+ __FILE__, __LINE__); \
+	} while (0)
+
+/*
+ *	TrapMacro is the same as Trap but it's intended for use in macros:
+ *
+ *		#define foo(x) (AssertMacro(x != 0), bar(x))
+ *
+ *	Isn't CPP fun?
+ */
+#define TrapMacro(condition, errorType) \
+	((bool) ((! assert_enabled) || ! (condition) || \
+			 (ExceptionalCondition(CppAsString(condition), (errorType), \
+   __FILE__, __LINE__), 0)))
+
+#define Assert(condition) \
+		Trap(!(condition), FailedAssertion)
+
+#define AssertMacro(condition) \
+		((void) TrapMacro(!(condition), FailedAssertion))
+
+#define AssertArg(condition) \
+		Trap(!(condition), BadArgument)
+
+#define AssertState(condition) \
+		Trap(!(condition), BadState)
+
+#endif /* USE_ASSERT_CHECKING  !FRONTEND */
+
+
+/*
  * Macros to support compile-time assertion checks.
  *
  * If the condition (a compile-time-constant expression) evaluates to false,
diff --git a/src/include/postgres.h b/src/include/postgres.h
index b6e922f..bbe125a 100644
--- a/src/include/postgres.h
+++ b/src/include/postgres.h
@@ -25,7 +25,7 @@
  *	  ---	
  *		1)		variable-length datatypes (TOAST support)
  *		2)		datum type + support macros
- *		3)		exception handling definitions
+ *		3)		exception handling
  *
  *	 NOTES
  *
@@ -627,62 +627,12 @@ extern Datum Float8GetDatum(float8 X);
 
 
 /* 
- *Section 3:	exception handling definitions
- *			Assert, Trap, etc macros
+ *Section 3:	exception handling backend support
  * 
  */
 
 extern PGDLLIMPORT bool assert_enabled;
 
-/*
- * USE_ASSERT_CHECKING, if defined, turns on all the assertions.
- * - plai  9/5/90
- *
- * It should _NOT_ be defined in releases or in benchmark copies
- */
-
-/*
- * Trap
- *		Generates an exception if the given condition is true.
- */
-#define Trap(condition, errorType) \
-	do { \
-		if ((assert_enabled)  (condition)) \
-			ExceptionalCondition(CppAsString(condition), (errorType), \
- __FILE__, __LINE__); \
-	} while (0)
-
-/*
- *	TrapMacro is the same as Trap but it's intended for use in macros:
- *
- *		#define foo(x) (AssertMacro(x != 0), bar(x))
- *
- *	Isn't CPP fun?
- */
-#define TrapMacro(condition, errorType) \
-	((bool) ((! assert_enabled) || ! (condition) || \
-			 (ExceptionalCondition(CppAsString(condition), (errorType), \
-   __FILE__, __LINE__), 0)))
-
-#ifndef USE_ASSERT_CHECKING
-#define Assert(condition)
-#define AssertMacro(condition)	((void)true)
-#define AssertArg(condition)
-#define AssertState(condition)
-#else
-#define Assert(condition) \
-		Trap(!(condition), FailedAssertion)
-
-#define AssertMacro(condition) \
-

Re: [HACKERS] HS locking broken in HEAD

On Thu, Jan 17, 2013 at 9:00 PM, Andres Freund and...@2ndquadrant.com wrote:
  I think it might also be a dangerous assumption for shared objects?

 Locks on shared objects can't be taken via the fast path.  In order to
 take a fast-path lock, a backend must be bound to a database and the
 locktag must be for a relation in that database.

 I assumed it wasn't allowed, but didn't find where that happens at first
 - I found it now though (c.f. SetLocktagRelationOid).

  A patch minimally addressing the is attached, but it only addresses part
  of the problem.

 Isn't the right fix to compare proc-databaseId to
 locktag-locktag_field1 rather than to MyDatabaseId?  The subsequent
 loop assumes that if the relid matches, the lock tag is an exact
 match, which will be correct with that rule but not the one you
 propose.

 I don't know much about the locking code, you're probably right. I also
 didn't look very far after finding the guilty commit.

 (reading code)

 Yes, I think you're right, that seems to be the actually correct fix.

 I am a bit worried that there are more such assumptions in the code, but
 I didn't find any, but I really don't know that code.

I found two instances of this.  See attached patch.

Can you test whether this fixes it for you?

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


fix-mydatabaseid-compare.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

Re: [HACKERS] Re: [PATCH 1/5] Centralize Assert* macros into c.h so its common between backend/frontend

Andres Freund wrote:
 On 2013-01-18 15:48:01 +0100, Andres Freund wrote:
  Here's a trivially updated patch which also defines AssertArg() for
  FRONTEND-ish environments since Alvaro added one in xlogreader.c.
 
 This time for real. Please.

Here's a different idea: move all the Assert() and StaticAssert() etc
definitions from c.h and postgres.h into a new header, say pgassert.h.
That header is included directly by postgres.h (just like palloc.h and
elog.h already are) so we don't have to touch the backend code at all.
Frontend programs that want that functionality can just #include
pgassert.h by themselves.  The definitions are (obviously) protected
by #ifdef FRONTEND so that it all works in both environments cleanly.

-- 
Álvaro Herrerahttp://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: [PATCH 1/5] Centralize Assert* macros into c.h so its common between backend/frontend

Alvaro Herrera alvhe...@2ndquadrant.com writes:
 Here's a different idea: move all the Assert() and StaticAssert() etc
 definitions from c.h and postgres.h into a new header, say pgassert.h.
 That header is included directly by postgres.h (just like palloc.h and
 elog.h already are) so we don't have to touch the backend code at all.
 Frontend programs that want that functionality can just #include
 pgassert.h by themselves.  The definitions are (obviously) protected
 by #ifdef FRONTEND so that it all works in both environments cleanly.

That might work.  Files using c.h would have to include this too, but
as described it should work in either environment for them.  The other
corner case is pg_controldata.c and other frontend programs that include
postgres.h --- but it looks like they #define FRONTEND first, so they'd
get the correct set of Assert definitions.

Whether it's really any better than just sticking them in c.h isn't
clear.

Really I'd prefer not to move the backend definitions out of postgres.h
at all, just because doing so will lose fifteen years of git history
about those particular lines (or at least make it a lot harder to
locate with git blame).

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: [PATCH 1/5] Centralize Assert* macros into c.h so its common between backend/frontend

Alvaro Herrera wrote:
 Andres Freund wrote:
  On 2013-01-18 15:48:01 +0100, Andres Freund wrote:
   Here's a trivially updated patch which also defines AssertArg() for
   FRONTEND-ish environments since Alvaro added one in xlogreader.c.
  
  This time for real. Please.
 
 Here's a different idea: move all the Assert() and StaticAssert() etc
 definitions from c.h and postgres.h into a new header, say pgassert.h.
 That header is included directly by postgres.h (just like palloc.h and
 elog.h already are) so we don't have to touch the backend code at all.
 Frontend programs that want that functionality can just #include
 pgassert.h by themselves.  The definitions are (obviously) protected
 by #ifdef FRONTEND so that it all works in both environments cleanly.

One slight problem with this is the common port/*.c idiom would require
an extra include:

#ifndef FRONTEND
#include postgres.h
#else
#include postgres_fe.h
#include pgassert.h   /* --- new line required here */
#endif /* FRONTEND */

If this is seen as a serious issue (I don't think so) then we would have
to include pgassert.h into postgres_fe.h which would make the whole
thing pointless (i.e. the same as just having the definitions in c.h).

-- 
Álvaro Herrerahttp://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] WIP patch for hint bit i/o mitigation

2013-01-18 Thread Merlin Moncure

On Fri, Jan 18, 2013 at 8:57 AM, Atri Sharma atri.j...@gmail.com wrote:

 Hello all,

 Sorry for the delay in updating the hackers list with the current status.

 I recently did some profiling using perf on PostgreSQL 9.2 with and without 
 our patch.

 I noticed that maximum time is being spent on heapgettup function. Further 
 investigation and a bit of a hunch leads me to believe that we may be 
 adversely affecting the visibility map optimisation that directly interact 
 with the visibility functions, that our patch straight away affects.

 If this is the case, we may really need to get down to the design of our 
 patch, and maybe see which visibility function/functions we are affecting, 
 and see if we can mitigate the affect.

 Please let me know your inputs on this.

Any scenario that involves non-trivial amount of investigation or
development should result in us pulling the patch for rework and
resubmission in later 'festit's closing time as they say :-).

merlin


-- 
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: [PATCH 1/5] Centralize Assert* macros into c.h so its common between backend/frontend

Alvaro Herrera alvhe...@2ndquadrant.com writes:
 One slight problem with this is the common port/*.c idiom would require
 an extra include:

 #ifndef FRONTEND
 #include postgres.h
 #else
 #include postgres_fe.h
 #include pgassert.h /* --- new line required here */
 #endif /* FRONTEND */

 If this is seen as a serious issue (I don't think so) then we would have
 to include pgassert.h into postgres_fe.h which would make the whole
 thing pointless (i.e. the same as just having the definitions in c.h).

We'd only need to add that when/if the file started to use asserts,
so I don't think it's a problem.  But still not sure it's better than
just putting the stuff in c.h.

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] Event Triggers: adding information

On 2013-01-18 09:58:53 -0500, Robert Haas wrote:
 On Fri, Jan 18, 2013 at 9:07 AM, Andres Freund and...@2ndquadrant.com wrote:
  I don't have a problem reusing the object access infrastructure at all. I 
  just
  don't think its providing even remotely enough. You have (co-)written that
  stuff, so you probably know more than I do, but could you explain to me how 
  it
  could be reused to replicate a CREATE TABLE?
 
  Problems I see:
  - afaics for CREATE TABLE the only hook is in ATExecAddColumn

 No, there's one also in heap_create_with_catalog.  Took me a minute to
 find it, as it does not use InvokeObjectAccessHook.  The idea is that
 OAT_POST_CREATE fires once per object creation, regardless of the
 object type - table, column, whatever.

Ah. Chose the wrong term to grep for. I am tempted to suggest adding a
comment explaining why we InvokeObjectAccessHook isn't used just for
enhanced grepability.

  - No way to regenerate the table definition for execution on the remote 
  system
without creating libpqdump.

 IMHO, that is one of the really ugly problems that we haven't come up
 with a good solution for yet.  If you want to replicate DDL, you have
 basically three choices:

 1. copy over the statement text that was used on the origin server and
 hope none of the corner cases bite you
 2. come up with some way of reconstituting a DDL statement based on
 (a) the parse tree or (b) what the server actually decided to do
 3. reconstitute the state of the object from the catalogs after the
 command has run

 (2a) differs from (2b) for things like CREATE INDEX, where the index
 name might be left for the server to determine, but when replicating
 you'd like to get the same name out.  (3) is workable for CREATE but
 not ALTER or DROP.

 The basic problem here is that (1) and (3) are not very
 reliable/complete and (2) is a lot of work and introduces a huge code
 maintenance burden.

I agree with that analysis.

 But it's unfair to pin that on the object-access
 hook mechanism

I agree. I don't think anybody has pinned it on it though.

 - any reverse-parsing or catalog-deconstruction
 solution for DDL is going to have that problem.  The decision we have
 to make as a community is whether we're prepared to support and
 maintain that code for the indefinite future.  Although I think it's
 easy to say yes, because DDL replication would be really cool - and
 I sure agree with that - I think it needs to be thought through a bit
 more deeply than that.

 I have been involved in PostgreSQL development for about 4.5 years
 now.  This is less time than many people here, but it's still long
 enough to say a whole lot of people ask for some variant of this idea,
 and yet I have yet to see anybody produce a complete, working version
 of this functionality and maintain it outside of the PostgreSQL tree
 for one release cycle (did I miss something?).

I don't really think thats a fair argument.

For one, I didn't really ask for a libpgdump - I said that I don't see
any way to generate re-executable SQL without it if we don't get the
parse-tree but only the oid of the created object. Not to speak of the
complexities of supporting ALTER that way (which is, as you note,
basically not the way to do this).

Also, even though I don't think its the right way *for this*, I think
pg_dump and pgadmin pretty much prove that it's possible?  The latter
even is out-of-core and has existed for multiple years.

Its also not really fair to compare out-of-tree effort of maintaining
such a library to in-core support. pg_dump *needs* to be maintained, so
the additional maintenance overhead once the initial development is done
shouldn't really be higher than now. Lower if anything, due to getting
rid of a good bit of ugly code ;). There's no such effect out of core.

If youre also considering parsetree-SQL transformation under the
libpgdump umbrella its even less fair. The backend already has a *lot*
of infrastructure to regenerate sql from trees, even if its mostly
centered arround around DQL. A noticeable amount of that code is
unavailable to extensions (i.e. many are static funcs).
Imo its completely unreasonable to expose that code to the outside and
expect it to have a stable interface. We *will* need to rewrite parts of
that regularly.
For those reasons I think the only reasonable way to create textual DDL
is in the backend trying to allow outside code to do that will impose
far greater limitations.

Whats the way you suggest to go on about this?

 There's a totally legitimate debate to be had here, but my feeling
 about what you're calling libpqdump is:

 - It's completely separate from event triggers.
 - It's completely separate from object access hooks.

Well, it is one of the major use-cases (or even *the* major use case)
for event triggers that I heard of. So it seems a valid point in a
dicussion on how to design the whole mechanism, doesn't it?

Some version of the event trigger patch contained partial support for

Re: [HACKERS] WIP patch for hint bit i/o mitigation

On Fri, Jan 18, 2013 at 10:36 AM, Merlin Moncure mmonc...@gmail.com wrote:
 Any scenario that involves non-trivial amount of investigation or
 development should result in us pulling the patch for rework and
 resubmission in later 'festit's closing time as they say :-).

Amen.

-- 
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] Re: [PATCH 1/5] Centralize Assert* macros into c.h so its common between backend/frontend

On 2013-01-18 10:33:16 -0500, Tom Lane wrote:
 Alvaro Herrera alvhe...@2ndquadrant.com writes:
  Here's a different idea: move all the Assert() and StaticAssert() etc
  definitions from c.h and postgres.h into a new header, say pgassert.h.
  That header is included directly by postgres.h (just like palloc.h and
  elog.h already are) so we don't have to touch the backend code at all.
  Frontend programs that want that functionality can just #include
  pgassert.h by themselves.  The definitions are (obviously) protected
  by #ifdef FRONTEND so that it all works in both environments cleanly.
 
 That might work.  Files using c.h would have to include this too, but
 as described it should work in either environment for them.  The other
 corner case is pg_controldata.c and other frontend programs that include
 postgres.h --- but it looks like they #define FRONTEND first, so they'd
 get the correct set of Assert definitions.
 
 Whether it's really any better than just sticking them in c.h isn't
 clear.

I don't really see an advantage. To me providing sensible asserts sounds
like a good fit for c.h... And we already have StaticAssert*,
AssertVariableIsOfType* in c.h...

 Really I'd prefer not to move the backend definitions out of postgres.h
 at all, just because doing so will lose fifteen years of git history
 about those particular lines (or at least make it a lot harder to
 locate with git blame).

I can imagine some ugly macro solutions to that problem (pgassert.h
includes postgres.h with special defines), but that doesn't seem to be
warranted to me.

The alternative seems to be sprinkling a few more #ifdef FRONTEND's in
postgres.h, if thats preferred I can prepare a patch for that although I
prefer my proposal.

Greetings,

Andres Freund

-- 
 Andres Freund 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] HS locking broken in HEAD

On 2013-01-18 10:15:20 -0500, Robert Haas wrote:
 On Thu, Jan 17, 2013 at 9:00 PM, Andres Freund and...@2ndquadrant.com wrote:
   I think it might also be a dangerous assumption for shared objects?
 
  Locks on shared objects can't be taken via the fast path.  In order to
  take a fast-path lock, a backend must be bound to a database and the
  locktag must be for a relation in that database.
 
  I assumed it wasn't allowed, but didn't find where that happens at first
  - I found it now though (c.f. SetLocktagRelationOid).
 
   A patch minimally addressing the is attached, but it only addresses part
   of the problem.
 
  Isn't the right fix to compare proc-databaseId to
  locktag-locktag_field1 rather than to MyDatabaseId?  The subsequent
  loop assumes that if the relid matches, the lock tag is an exact
  match, which will be correct with that rule but not the one you
  propose.
 
  I don't know much about the locking code, you're probably right. I also
  didn't look very far after finding the guilty commit.
 
  (reading code)
 
  Yes, I think you're right, that seems to be the actually correct fix.
 
  I am a bit worried that there are more such assumptions in the code, but
  I didn't find any, but I really don't know that code.
 
 I found two instances of this.  See attached patch.

Yea, those are the two sites I had fixed (incorrectly) as well. I
looked a bit more yesterday night and I didn't find any additional
locations, so I hope we got this.

Yep, seems to work.

I am still stupefied nobody noticed that locking in HS (where just about
all locks are going to be fast path locks) was completely broken for
nearly two years.

Greetings,

Andres Freund

-- 
 Andres Freund 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: [PATCH 1/5] Centralize Assert* macros into c.h so its common between backend/frontend

Andres Freund and...@2ndquadrant.com writes:
 On 2013-01-18 10:33:16 -0500, Tom Lane wrote:
 Really I'd prefer not to move the backend definitions out of postgres.h
 at all, just because doing so will lose fifteen years of git history
 about those particular lines (or at least make it a lot harder to
 locate with git blame).

 The alternative seems to be sprinkling a few more #ifdef FRONTEND's in
 postgres.h, if thats preferred I can prepare a patch for that although I
 prefer my proposal.

Yeah, surrounding the existing definitions with #ifndef FRONTEND was
what I was imagining.  But on reflection that seems pretty darn ugly,
especially if the corresponding FRONTEND definitions are far away.
Maybe we should just live with the git history disconnect.

Right at the moment the c.h location seems like the best bet.  I don't
see much advantage to Alvaro's proposal of a new header file.

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] Re: [PATCH 1/5] Centralize Assert* macros into c.h so its common between backend/frontend

On 2013-01-18 11:11:50 -0500, Tom Lane wrote:
 Andres Freund and...@2ndquadrant.com writes:
  On 2013-01-18 10:33:16 -0500, Tom Lane wrote:
  Really I'd prefer not to move the backend definitions out of postgres.h
  at all, just because doing so will lose fifteen years of git history
  about those particular lines (or at least make it a lot harder to
  locate with git blame).
 
  The alternative seems to be sprinkling a few more #ifdef FRONTEND's in
  postgres.h, if thats preferred I can prepare a patch for that although I
  prefer my proposal.
 
 Yeah, surrounding the existing definitions with #ifndef FRONTEND was
 what I was imagining.  But on reflection that seems pretty darn ugly,
 especially if the corresponding FRONTEND definitions are far away.
 Maybe we should just live with the git history disconnect.

FWIW, git blame -M -C, while noticeably more expensive, seems to be able to
connect the history:

d08741ea src/include/postgres.h(Tom Lane   2001-02-10 
02:31:31 +  746) 
d08741ea src/include/postgres.h(Tom Lane   2001-02-10 
02:31:31 +  747) #define Assert(condition) \
c5354dff src/include/postgres.h(Bruce Momjian  2002-08-10 
20:29:18 +  748)  Trap(!(condition), FailedAssertion)
d08741ea src/include/postgres.h(Tom Lane   2001-02-10 
02:31:31 +  749) 
d08741ea src/include/postgres.h(Tom Lane   2001-02-10 
02:31:31 +  750) #define AssertMacro(condition) \
c5354dff src/include/postgres.h(Bruce Momjian  2002-08-10 
20:29:18 +  751)  ((void) TrapMacro(!(condition), 
FailedAssertion))
d08741ea src/include/postgres.h(Tom Lane   2001-02-10 
02:31:31 +  752) 
d08741ea src/include/postgres.h(Tom Lane   2001-02-10 
02:31:31 +  753) #define AssertArg(condition) \
c5354dff src/include/postgres.h(Bruce Momjian  2002-08-10 
20:29:18 +  754)  Trap(!(condition), BadArgument)
d08741ea src/include/postgres.h(Tom Lane   2001-02-10 
02:31:31 +  755) 
d08741ea src/include/postgres.h(Tom Lane   2001-02-10 
02:31:31 +  756) #define AssertState(condition) \
c5354dff src/include/postgres.h(Bruce Momjian  2002-08-10 
20:29:18 +  757)  Trap(!(condition), BadState)
8118de2b src/include/c.h   (Andres Freund  2012-12-18 
00:58:36 +0100  758) 


Andres

-- 
 Andres Freund 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] HS locking broken in HEAD

Andres Freund and...@2ndquadrant.com writes:
 I am still stupefied nobody noticed that locking in HS (where just about
 all locks are going to be fast path locks) was completely broken for
 nearly two years.

IIUC it would only be broken for cases where activity was going on
concurrently in two different databases, which maybe isn't all that
common out in the field.  And for sure it isn't something we test much.

I wonder if it'd be practical to, say, run all the contrib regression
tests concurrently in different databases of one installation.

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] HS locking broken in HEAD

On 2013-01-18 11:16:15 -0500, Tom Lane wrote:
 Andres Freund and...@2ndquadrant.com writes:
  I am still stupefied nobody noticed that locking in HS (where just about
  all locks are going to be fast path locks) was completely broken for
  nearly two years.
 
 IIUC it would only be broken for cases where activity was going on
 concurrently in two different databases, which maybe isn't all that
 common out in the field.  And for sure it isn't something we test much.

I think effectively it only was broken in Hot Standby. At least I don't
immediately can think of a scenario where a strong lock is being acquired on a
non-shared object in a different database.

 I wonder if it'd be practical to, say, run all the contrib regression
 tests concurrently in different databases of one installation.

I think it would be a good idea, but I don't immediately have an idea
how to implement it. It seems to me we would need to put the logic for
it into pg_regress? Otherwise the lifetime management of the shared
postmaster seems to get complicated.

What I would really like is to have some basic replication test scenario
in the regression tests. That seems like a dramatically undertested, but
pretty damn essential, part of the code.

The first handwavy guess I have of doing it is something like connecting
a second postmaster to the primary one at the start of the main
regression tests (requires changing the wal level again, yuck) and
fuzzyly comparing a pg_dump of the database remnants in both clusters at
the end of the regression tests.

Better ideas?

Greetings,

Andres Freund

-- 
 Andres Freund 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] HS locking broken in HEAD

On 2013-01-18 17:26:00 +0100, Andres Freund wrote:
 On 2013-01-18 11:16:15 -0500, Tom Lane wrote:
  Andres Freund and...@2ndquadrant.com writes:
   I am still stupefied nobody noticed that locking in HS (where just about
   all locks are going to be fast path locks) was completely broken for
   nearly two years.
  
  IIUC it would only be broken for cases where activity was going on
  concurrently in two different databases, which maybe isn't all that
  common out in the field.  And for sure it isn't something we test much.
 
 I think effectively it only was broken in Hot Standby. At least I don't
 immediately can think of a scenario where a strong lock is being acquired on a
 non-shared object in a different database.

Hrmpf, should have mentioned that the problem in HS is that the startup
process is doing exactly that, which is why it is broke there. It
acquires the exclusive locks shipped via WAL so the following
non-concurrency safe actions can be applied. And obviously its not
connected to any database while doing it...

I would have guessed its not that infrequent to run an ALTER TABLE or
somesuch while the standby is still running some longrunning query...

Greetings,

Andres Freund

-- 
 Andres Freund 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] logical changeset generation v4

Andres Freund wrote:

 [09] Adjust all *Satisfies routines to take a HeapTuple instead of a 
 HeapTupleHeader
 
 For timetravel access to the catalog we need to be able to lookup (cmin,
 cmax) pairs of catalog rows when were 'inside' that TX. This patch just
 adapts the signature of the *Satisfies routines to expect a HeapTuple
 instead of a HeapTupleHeader. The amount of changes for that is fairly
 low as the HeapTupleSatisfiesVisibility macro already expected the
 former.
 
 It also makes sure the HeapTuple fields are setup in the few places that
 didn't already do so.

I had a look at this part.  Running the regression tests unveiled a case
where the tableOid wasn't being set (and thus caused an assertion to
fail), so I added that.  I also noticed that the additions to
pruneheap.c are sometimes filling a tuple before it's strictly
necessary, leading to wasted work.  Moved those too.

Looks good to me as attached.

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training  Services
*** a/contrib/pgrowlocks/pgrowlocks.c
--- b/contrib/pgrowlocks/pgrowlocks.c
***
*** 120,126  pgrowlocks(PG_FUNCTION_ARGS)
  		/* must hold a buffer lock to call HeapTupleSatisfiesUpdate */
  		LockBuffer(scan-rs_cbuf, BUFFER_LOCK_SHARE);
  
! 		if (HeapTupleSatisfiesUpdate(tuple-t_data,
  	 GetCurrentCommandId(false),
  	 scan-rs_cbuf) == HeapTupleBeingUpdated)
  		{
--- 120,126 
  		/* must hold a buffer lock to call HeapTupleSatisfiesUpdate */
  		LockBuffer(scan-rs_cbuf, BUFFER_LOCK_SHARE);
  
! 		if (HeapTupleSatisfiesUpdate(tuple,
  	 GetCurrentCommandId(false),
  	 scan-rs_cbuf) == HeapTupleBeingUpdated)
  		{
*** a/src/backend/access/heap/heapam.c
--- b/src/backend/access/heap/heapam.c
***
*** 291,296  heapgetpage(HeapScanDesc scan, BlockNumber page)
--- 291,297 
  
  			loctup.t_data = (HeapTupleHeader) PageGetItem((Page) dp, lpp);
  			loctup.t_len = ItemIdGetLength(lpp);
+ 			loctup.t_tableOid = RelationGetRelid(scan-rs_rd);
  			ItemPointerSet((loctup.t_self), page, lineoff);
  
  			if (all_visible)
***
*** 1603,1609  heap_hot_search_buffer(ItemPointer tid, Relation relation, Buffer buffer,
  
  		heapTuple-t_data = (HeapTupleHeader) PageGetItem(dp, lp);
  		heapTuple-t_len = ItemIdGetLength(lp);
! 		heapTuple-t_tableOid = relation-rd_id;
  		heapTuple-t_self = *tid;
  
  		/*
--- 1604,1610 
  
  		heapTuple-t_data = (HeapTupleHeader) PageGetItem(dp, lp);
  		heapTuple-t_len = ItemIdGetLength(lp);
! 		heapTuple-t_tableOid = RelationGetRelid(relation);
  		heapTuple-t_self = *tid;
  
  		/*
***
*** 1651,1657  heap_hot_search_buffer(ItemPointer tid, Relation relation, Buffer buffer,
  		 * transactions.
  		 */
  		if (all_dead  *all_dead 
! 			!HeapTupleIsSurelyDead(heapTuple-t_data, RecentGlobalXmin))
  			*all_dead = false;
  
  		/*
--- 1652,1658 
  		 * transactions.
  		 */
  		if (all_dead  *all_dead 
! 			!HeapTupleIsSurelyDead(heapTuple, RecentGlobalXmin))
  			*all_dead = false;
  
  		/*
***
*** 1781,1786  heap_get_latest_tid(Relation relation,
--- 1782,1788 
  		tp.t_self = ctid;
  		tp.t_data = (HeapTupleHeader) PageGetItem(page, lp);
  		tp.t_len = ItemIdGetLength(lp);
+ 		tp.t_tableOid = RelationGetRelid(relation);
  
  		/*
  		 * After following a t_ctid link, we might arrive at an unrelated
***
*** 2447,2458  heap_delete(Relation relation, ItemPointer tid,
  	lp = PageGetItemId(page, ItemPointerGetOffsetNumber(tid));
  	Assert(ItemIdIsNormal(lp));
  
  	tp.t_data = (HeapTupleHeader) PageGetItem(page, lp);
  	tp.t_len = ItemIdGetLength(lp);
  	tp.t_self = *tid;
  
  l1:
! 	result = HeapTupleSatisfiesUpdate(tp.t_data, cid, buffer);
  
  	if (result == HeapTupleInvisible)
  	{
--- 2449,2461 
  	lp = PageGetItemId(page, ItemPointerGetOffsetNumber(tid));
  	Assert(ItemIdIsNormal(lp));
  
+ 	tp.t_tableOid = RelationGetRelid(relation);
  	tp.t_data = (HeapTupleHeader) PageGetItem(page, lp);
  	tp.t_len = ItemIdGetLength(lp);
  	tp.t_self = *tid;
  
  l1:
! 	result = HeapTupleSatisfiesUpdate(tp, cid, buffer);
  
  	if (result == HeapTupleInvisible)
  	{
***
*** 2817,2822  heap_update(Relation relation, ItemPointer otid, HeapTuple newtup,
--- 2820,2826 
  	lp = PageGetItemId(page, ItemPointerGetOffsetNumber(otid));
  	Assert(ItemIdIsNormal(lp));
  
+ 	oldtup.t_tableOid = RelationGetRelid(relation);
  	oldtup.t_data = (HeapTupleHeader) PageGetItem(page, lp);
  	oldtup.t_len = ItemIdGetLength(lp);
  	oldtup.t_self = *otid;
***
*** 2829,2835  heap_update(Relation relation, ItemPointer otid, HeapTuple newtup,
  	 */
  
  l2:
! 	result = HeapTupleSatisfiesUpdate(oldtup.t_data, cid, buffer);
  
  	if (result == HeapTupleInvisible)
  	{
--- 2833,2839 
  	 */
  
  l2:
! 	result = HeapTupleSatisfiesUpdate(oldtup, cid, buffer);
  
  	if (result == HeapTupleInvisible)
  	{

Re: [HACKERS] Event Triggers: adding information

On Fri, Jan 18, 2013 at 10:47 AM, Andres Freund and...@2ndquadrant.com wrote:
 No, there's one also in heap_create_with_catalog.  Took me a minute to
 find it, as it does not use InvokeObjectAccessHook.  The idea is that
 OAT_POST_CREATE fires once per object creation, regardless of the
 object type - table, column, whatever.

 Ah. Chose the wrong term to grep for. I am tempted to suggest adding a
 comment explaining why we InvokeObjectAccessHook isn't used just for
 enhanced grepability.

I cannot parse that sentence, but I agree that the ungreppability of
it is suboptimal.  I resorted to git log -SInvokeObjectAccessHook -p
to find it.

 I have been involved in PostgreSQL development for about 4.5 years
 now.  This is less time than many people here, but it's still long
 enough to say a whole lot of people ask for some variant of this idea,
 and yet I have yet to see anybody produce a complete, working version
 of this functionality and maintain it outside of the PostgreSQL tree
 for one release cycle (did I miss something?).

 I don't really think thats a fair argument.

 For one, I didn't really ask for a libpgdump - I said that I don't see
 any way to generate re-executable SQL without it if we don't get the
 parse-tree but only the oid of the created object. Not to speak of the
 complexities of supporting ALTER that way (which is, as you note,
 basically not the way to do this).

Oh, OK.  I see.  Well, I've got no problem making the parse tree
available via InvokeObjectAccessHook.  Isn't that just a matter of
adding a new hook type and a new call site?

 Also, even though I don't think its the right way *for this*, I think
 pg_dump and pgadmin pretty much prove that it's possible?  The latter
 even is out-of-core and has existed for multiple years.

Fair point.

 Its also not really fair to compare out-of-tree effort of maintaining
 such a library to in-core support. pg_dump *needs* to be maintained, so
 the additional maintenance overhead once the initial development is done
 shouldn't really be higher than now. Lower if anything, due to getting
 rid of a good bit of ugly code ;). There's no such effect out of core.

This I don't follow.  Nothing we might add to core to reverse-parse
either the catalogs or the parse tree is going to make pg_dump go
away.

 If youre also considering parsetree-SQL transformation under the
 libpgdump umbrella its even less fair. The backend already has a *lot*
 of infrastructure to regenerate sql from trees, even if its mostly
 centered arround around DQL. A noticeable amount of that code is
 unavailable to extensions (i.e. many are static funcs).
 Imo its completely unreasonable to expose that code to the outside and
 expect it to have a stable interface. We *will* need to rewrite parts of
 that regularly.
 For those reasons I think the only reasonable way to create textual DDL
 is in the backend trying to allow outside code to do that will impose
 far greater limitations.

I'm having trouble following this.  Can you restate?  I wasn't sure
what you meant by libpqdump.  I assumed you were speaking of a
parsetree-DDL or catalog-DDL facility.

 Well, it is one of the major use-cases (or even *the* major use case)
 for event triggers that I heard of. So it seems a valid point in a
 dicussion on how to design the whole mechanism, doesn't it?

Yes, but is a separate problem from how we give control to the event
trigger (or object access hook).

 Some version of the event trigger patch contained partial support for
 regenerating the DDL so it seems like a justified point there. Your
 complained that suggestions about reusing object access hooks were
 ignored, so mentioning that they currently don't provide *enough* (they
 *do* provide a part, but it doesn't seem to be the most critical one)
 also seems justifyable.

Sure, but we could if we wanted decide to commit the partial support
for regenerating the DDL and tell people to use it via object access
hooks.  Of course, that would thin out even further the number of
people who would actually be using that code, which I fear will
already be too small to achieve bug-free operation in a reasonable
time.  If we add some hooks now and someone maintains the DDL
reverse-parsing code as an out-of-core replication solution for a few
releases, and that doesn't turn out to be hideous, I'd be a lot more
sanguine about incorporating it at that point.  I basically don't
think that there's any way we're going to commit something along those
lines now and not have it turn out to be a far bigger maintenance
headache than anyone wants.  What that tends to end up meaning in
practice is that Tom gets suckered into fixing it because nobody else
can take the time, and that's neither fair nor good for the project.

 NP, its good to keep the technical stuff here anyway... Stupid
 technology. In which business are we in again?

I don't know, maybe we can figure it out based on the objects in our
immediate vicinity.  I see twelve cans of paint, most

Re: [HACKERS] logical changeset generation v4

Alvaro Herrera wrote:

 I had a look at this part.  Running the regression tests unveiled a case
 where the tableOid wasn't being set (and thus caused an assertion to
 fail), so I added that.  I also noticed that the additions to
 pruneheap.c are sometimes filling a tuple before it's strictly
 necessary, leading to wasted work.  Moved those too.

Actually I missed that downthread there are some fixes to this part; I
had fixed one of these independently, but there's one I missed.  Added
that one too now (not attaching a new version).

(Also, it seems pointless to commit this unless we know for sure that
the downstream change that requires it is good; so I'm holding commit
until we've discussed the other stuff more thoroughly.  Per discussion
with Andres.)

-- 
Álvaro Herrerahttp://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] logical changeset generation v4

On Fri, Jan 18, 2013 at 11:33 AM, Alvaro Herrera
alvhe...@2ndquadrant.com wrote:
 Andres Freund wrote:

 [09] Adjust all *Satisfies routines to take a HeapTuple instead of a 
 HeapTupleHeader

 For timetravel access to the catalog we need to be able to lookup (cmin,
 cmax) pairs of catalog rows when were 'inside' that TX. This patch just
 adapts the signature of the *Satisfies routines to expect a HeapTuple
 instead of a HeapTupleHeader. The amount of changes for that is fairly
 low as the HeapTupleSatisfiesVisibility macro already expected the
 former.

 It also makes sure the HeapTuple fields are setup in the few places that
 didn't already do so.

 I had a look at this part.  Running the regression tests unveiled a case
 where the tableOid wasn't being set (and thus caused an assertion to
 fail), so I added that.  I also noticed that the additions to
 pruneheap.c are sometimes filling a tuple before it's strictly
 necessary, leading to wasted work.  Moved those too.

 Looks good to me as attached.

I took a quick look at this and am just curious why we're adding the
requirement that t_tableOid has to be initialized?

-- 
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] proposal: fix corner use case of variadic fuctions usage

Stephen Frost sfr...@snowman.net writes:
 [ quick review of patch ]

On reflection it seems to me that this is probably not a very good
approach overall.  Our general theory for functions taking ANY has
been that the core system just computes the arguments and leaves it
to the function to make sense of them.  Why should this be different
in the one specific case of VARIADIC ANY with a variadic array?

The approach is also inherently seriously inefficient.  Not only
does ExecMakeFunctionResult have to build a separate phony Const
node for each array element, but the variadic function itself can
have no idea that those Consts are all the same type, which means
it's going to do datatype lookup over again for each array element.
(format(), for instance, would have to look up the same type output
function over and over again.)  This might not be noticeable on toy
examples with a couple of array entries, but it'll be a large
performance problem on large arrays.

The patch also breaks any attempt that a variadic function might be
making to cache info about its argument types across calls.  I suppose
that the copying of the FmgrInfo is a hack to avoid crashes due to
called functions supposing that their flinfo-fn_extra data is still
valid for the new argument set.  Of course that's another pretty
significant performance hit, compounding the per-element hit.  Whereas
an ordinary variadic function that is given N arguments in a particular
query will probably do N datatype lookups and cache the info for the
life of the query, a variadic function called with this approach will
do one datatype lookup for each array element in each row of the query;
and there is no way to optimize that.

But large arrays have a worse problem: the approach flat out does
not work for arrays of more than FUNC_MAX_ARGS elements, because
there's no place to put their values in the FunctionCallInfo struct.
(As submitted, if the array is too big the patch will blithely stomp
all over memory with user-supplied data, making it not merely a crash
risk but probably an exploitable security hole.)

This last problem is, so far as I can see, unfixable within this
approach; surely we are not going to accept a feature that only works
for small arrays.  So I am going to mark the CF item rejected not just
RWF.

I believe that a workable approach would require having the function
itself act differently when the VARIADIC flag is set.  Currently there
is no way for ANY-taking functions to do this because we don't record
the use of the VARIADIC flag in FuncExpr, but it'd be reasonable to
change that, and probably then add a function similar to
get_fn_expr_rettype to dig it out of the FmgrInfo.  I don't know if
we could usefully provide any infrastructure beyond that for the case,
but it'd be worth thinking about whether there's any common behavior
for such functions.

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] Event Triggers: adding information

On 2013-01-18 11:42:47 -0500, Robert Haas wrote:
 On Fri, Jan 18, 2013 at 10:47 AM, Andres Freund and...@2ndquadrant.com 
 wrote:
  No, there's one also in heap_create_with_catalog.  Took me a minute to
  find it, as it does not use InvokeObjectAccessHook.  The idea is that
  OAT_POST_CREATE fires once per object creation, regardless of the
  object type - table, column, whatever.
 
  Ah. Chose the wrong term to grep for. I am tempted to suggest adding a
  comment explaining why we InvokeObjectAccessHook isn't used just for
  enhanced grepability.
 
 I cannot parse that sentence, but I agree that the ungreppability of
 it is suboptimal.  I resorted to git log -SInvokeObjectAccessHook -p
 to find it.

I was thinking of adding a comment like
/* We don't use InvokeObjectAccessHook here because we need to ... */
not because the comment in itself is important but because it allows to
grep ;)


  I have been involved in PostgreSQL development for about 4.5 years
  now.  This is less time than many people here, but it's still long
  enough to say a whole lot of people ask for some variant of this idea,
  and yet I have yet to see anybody produce a complete, working version
  of this functionality and maintain it outside of the PostgreSQL tree
  for one release cycle (did I miss something?).
 
  I don't really think thats a fair argument.
 
  For one, I didn't really ask for a libpgdump - I said that I don't see
  any way to generate re-executable SQL without it if we don't get the
  parse-tree but only the oid of the created object. Not to speak of the
  complexities of supporting ALTER that way (which is, as you note,
  basically not the way to do this).
 
 Oh, OK.  I see.  Well, I've got no problem making the parse tree
 available via InvokeObjectAccessHook.  Isn't that just a matter of
 adding a new hook type and a new call site?

Sure. Its probably not easy to choose the correct callsites though, they
need to be somewhat different for create  alter in comparison to drop.

create  alter
* after the object is created/altered
drop:
* after the object is locked, but *before* its dropped

As far as I understood thats basically the behind the ddl_trace event
trigger mentioned earlier.

  Also, even though I don't think its the right way *for this*, I think
  pg_dump and pgadmin pretty much prove that it's possible?  The latter
  even is out-of-core and has existed for multiple years.
 
 Fair point.

Imo its already a good justification for a libpgdump, not that I am
volunteering, but ...

  Its also not really fair to compare out-of-tree effort of maintaining
  such a library to in-core support. pg_dump *needs* to be maintained, so
  the additional maintenance overhead once the initial development is done
  shouldn't really be higher than now. Lower if anything, due to getting
  rid of a good bit of ugly code ;). There's no such effect out of core.
 
 This I don't follow.  Nothing we might add to core to reverse-parse
 either the catalogs or the parse tree is going to make pg_dump go
 away.

I was still talking about the hypothetical libpgdump we don't really
need for this ;)

  If youre also considering parsetree-SQL transformation under the
  libpgdump umbrella its even less fair. The backend already has a *lot*
  of infrastructure to regenerate sql from trees, even if its mostly
  centered arround around DQL. A noticeable amount of that code is
  unavailable to extensions (i.e. many are static funcs).
  Imo its completely unreasonable to expose that code to the outside and
  expect it to have a stable interface. We *will* need to rewrite parts of
  that regularly.
  For those reasons I think the only reasonable way to create textual DDL
  is in the backend trying to allow outside code to do that will impose
  far greater limitations.
 
 I'm having trouble following this.  Can you restate?  I wasn't sure
 what you meant by libpqdump.  I assumed you were speaking of a
 parsetree-DDL or catalog-DDL facility.

Yea, that wasn't really clear, sorry for that.

What I was trying to say that I think doing parstree-text conversion
out of tree is noticeably more complex and essentially places a higher
maintenance burden on the project because
1) the core already has a lot of infrastructure for such conversions
2) any external project would either need to copy that infrastructure or
   make a large number of functions externally visible
3) any refactoring in that area would now break external tools even if
   it would be trivial to adjust potential in-core support for such a
   conversion

  Some version of the event trigger patch contained partial support for
  regenerating the DDL so it seems like a justified point there. Your
  complained that suggestions about reusing object access hooks were
  ignored, so mentioning that they currently don't provide *enough* (they
  *do* provide a part, but it doesn't seem to be the most critical one)
  also seems justifyable.
 
 Sure, but we could if we wanted decide to commit the

Re: [HACKERS] review: pgbench - aggregation of info written into log

On Thu, Jan 17, 2013 at 7:43 PM, Tatsuo Ishii is...@postgresql.org wrote:
 So if my understating is correct, 1)Tomas Vondra commits to work on
 Windows support for 9.4, 2)on the assumption that one of Andrew
 Dunstan, Dave Page or Magnus Hagander will help him in Windows
 development.

 Ok? If so, I can commit the patch for 9.3 without Windows support. If
 not, I will move the patch to next CF (for 9.4).

 Please correct me if I am wrong.

+1 for this approach.  I agree with Dave and Magnus that we don't want
Windows to become a second-class platform, but this patch isn't making
it so.  The #ifdef that peeks inside of an instr_time is already
there, and it's not Tomas's fault that nobody has gotten around to
fixing it before now.

OTOH, I think that this sort of thing is quite wrong:

+#ifndef WIN32
+--aggregate-interval NUM\n
+ aggregate data over NUM seconds\n
+#endif

The right approach if this can't be supported on Windows is to still
display the option in the --help output, and to display an error
message if the user tries to use it, saying that it is not currently
supported on Windows.  That fact should also be mentioned in the
documentation.

-- 
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] HS locking broken in HEAD

Andres Freund and...@2ndquadrant.com writes:
 On 2013-01-18 11:16:15 -0500, Tom Lane wrote:
 I wonder if it'd be practical to, say, run all the contrib regression
 tests concurrently in different databases of one installation.

 I think it would be a good idea, but I don't immediately have an idea
 how to implement it. It seems to me we would need to put the logic for
 it into pg_regress? Otherwise the lifetime management of the shared
 postmaster seems to get complicated.

Seems like it'd be sufficient to make it work for the make
installcheck case, which dodges the postmaster problem.

 What I would really like is to have some basic replication test scenario
 in the regression tests.

Agreed, that's not being tested enough either.

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] logical changeset generation v4

On 2013-01-18 11:48:43 -0500, Robert Haas wrote:
 On Fri, Jan 18, 2013 at 11:33 AM, Alvaro Herrera
 alvhe...@2ndquadrant.com wrote:
  Andres Freund wrote:
 
  [09] Adjust all *Satisfies routines to take a HeapTuple instead of a 
  HeapTupleHeader
 
  For timetravel access to the catalog we need to be able to lookup (cmin,
  cmax) pairs of catalog rows when were 'inside' that TX. This patch just
  adapts the signature of the *Satisfies routines to expect a HeapTuple
  instead of a HeapTupleHeader. The amount of changes for that is fairly
  low as the HeapTupleSatisfiesVisibility macro already expected the
  former.
 
  It also makes sure the HeapTuple fields are setup in the few places that
  didn't already do so.
 
  I had a look at this part.  Running the regression tests unveiled a case
  where the tableOid wasn't being set (and thus caused an assertion to
  fail), so I added that.  I also noticed that the additions to
  pruneheap.c are sometimes filling a tuple before it's strictly
  necessary, leading to wasted work.  Moved those too.
 
  Looks good to me as attached.

 I took a quick look at this and am just curious why we're adding the
 requirement that t_tableOid has to be initialized?

Its a stepping stone for catalog timetravel. I separated it into a different
patch because it seems to make the real patch easier to review without having
to deal with all those unrelated hunks.

The reason why we need t_tableOid and a valid ItemPointer is that during
catalog timetravel (so we can decode the heaptuples in WAL) we need to
see tuples in the catalog that have been changed in the transaction we
travelled to. That means we need to lookup cmin/cmax values which aren't
stored separately anymore.

My first approach was to build support for logging allocated combocids
(only for catalog tables) and use the existing combocid infrastructure
to look them up.
Turns out thats not a correct solution, consider this:
* T100: INSERT (xmin: 100, xmax: Invalid, (cmin|cmax): 3)
* T101: UPDATE (xmin: 100, xmax: 101, (cmin|cmax): 10)

If you know travel to T100 and you want to decide whether that tuple is
visible when in CommandId = 5 you have the problem that the original
cmin value has been overwritten by the cmax from T101. Note that in this
scenario no ComboCids have been generated!
The problematic part is that the information about what happened is
only available in T101.

I took resolve to doing something similar to what the heap rewrite code
uses to track update chains. Everytime a catalog tuple
inserted/updated/deleted (filenode, ctid, cmin, cmax) is wal logged (if
wal_level=logical) and while traveling to a transaction all those are
put up in a hash table so they can get looked up if we need the
respective cmin/cmax values. As we do that for all modifications of
catalog tuples in that transaction we only ever need that mapping when
inspecting that specific transaction.

Seems to work very nicely, I have made quite some tests with it and I
know of no failure cases.


To be able to make that lookup we need to get the relfilenode  item
pointer of the tuple were just looking up. Thats why I changed the
signature to pass a HeapTuple instead of a HeapTupleHeader. We get the
relfilenode from the buffer that has been passed *not* from the passed
table oid.
So requiring a valid table oid isn't strictly required as long as the
item pointer is valid, but it has made debugging noticeably easier.

Makes sense?

Greetings,

Andres Freund

--
 Andres Freund 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] logical changeset generation v4

Robert Haas robertmh...@gmail.com writes:
 I took a quick look at this and am just curious why we're adding the
 requirement that t_tableOid has to be initialized?

I assume he meant it had been left at a random value, which is surely
bad practice even if a specific usage doesn't fall over 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

Re: [HACKERS] Event Triggers: adding information

Andres Freund and...@2ndquadrant.com writes:
 On 2013-01-18 11:42:47 -0500, Robert Haas wrote:
 I'm having trouble following this.  Can you restate?  I wasn't sure
 what you meant by libpqdump.  I assumed you were speaking of a
 parsetree-DDL or catalog-DDL facility.

 Yea, that wasn't really clear, sorry for that.

 What I was trying to say that I think doing parstree-text conversion
 out of tree is noticeably more complex and essentially places a higher
 maintenance burden on the project because

Ah.  That's not pg_dump's job though, so you're choosing a bad name for
it.  What I think you are really talking about is extending the
deparsing logic in ruleutils.c to cover utility statements.  Which is
not a bad plan in principle; we've just never needed it before.  It
would be quite a lot of new code though.

An issue that would have to be thought about is that there are assorted
scenarios where we generate parse trees that contain options that
cannot be generated from SQL, so there's no way to reverse-list them
into working SQL.  But often those hidden options are essential to know
what the statement will really do, so I'm not sure ignoring them will be
a workable answer for replication purposes.

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] in-catalog Extension Scripts and Control parameters (templates?)

* Dimitri Fontaine (dimi...@2ndquadrant.fr) wrote:
 Please find attached a preliminary patch following the TEMPLATE ideas,
 and thanks in particular to Tom and Heikki for a practical design about
 how to solve that problem!

Given that it's preliminary and v0 and big and whatnot, it seems like
it should be bounced to post-9.3.  Even so, I did take a look through
it, probably mostly because I'd really like to see this feature. :)

What's with removing the OBJECT_TABLESPACE case?  Given that
get_object_address_tmpl() seems to mainly just fall into a couple of
case statements to split out the different options, I'm not sure that
having that function is useful, or perhaps it should be 2 distinct
functions?

ExtensionControlFile seemed like a good name, just changing that to
ExtensionControl doesn't seem as nice, tho that's a bit of bike
shedding, I suppose.

I'm not sure we have a 'dile system'... :)

For my 2c, I wish we could do something better than having to support
both on-disk conf files and in-database configs.  Don't have any
particular solution to that tho.

Also pretty sure we only have one catalog
('get_ext_ver_list_from_catalogs')

'Template' seems like a really broad term which might end up being
associated with things beyond extensions, yet there are a number of
places where you just use 'TEMPLATE', eg, ACL_KIND_TEMPLATE.  Seems like
it might become an issue later.

Just a side-note, there's also some whitespace issues.

Also, no pg_dump/restore support..?  Seems like that'd be useful..

That's just a real quick run-through with my notes.  If this patch is
really gonna go into 9.3, I'll try to take a deeper look.

Thanks,

Stephen


signature.asc
Description: Digital signature

[HACKERS] Inconsistent format() behavior for argument-count inconsistency

regression=# select format('%s %s', 'foo', 'bar');
 format  
-
 foo bar
(1 row)

regression=# select format('%s %s', 'foo', 'bar', 'baz');
 format  
-
 foo bar
(1 row)

regression=# select format('%s %s', 'foo');  
ERROR:  too few arguments for format

Why do we throw an error for too few arguments, but not too many?

(This came up when I started wondering whether the proposed VARIADIC
feature would really be very useful for format(), since it needs a
format string that matches up with its arguments.)

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] Event Triggers: adding information

On 2013-01-18 12:44:13 -0500, Tom Lane wrote:
Andres Freund and...@2ndquadrant.com writes:
On 2013-01-18 11:42:47 -0500, Robert Haas wrote:
I'm having trouble following this. Can you restate? I wasn't sure
what you meant by libpqdump. I assumed you were speaking of a
parsetree-DDL or catalog-DDL facility.

Yea, that wasn't really clear, sorry for that.

What I was trying to say that I think doing parstree-text conversion
out of tree is noticeably more complex and essentially places a higher
maintenance burden on the project because

Ah. That's not pg_dump's job though, so you're choosing a bad name for
it.

I really only mentioned libpgdump because the object access hooks at the
moment only get passed the object oid and because Robert doubted the
need for deparsing the parsetrees in the past. Without the parsetree you
obviousl cannot deparse the parsetree ;)

I do *not* think that using something that reassembles the statement
solely based on the catalog context is a good idea.

What I think you are really talking about is extending the
deparsing logic in ruleutils.c to cover utility statements. Which is
not a bad plan in principle; we've just never needed it before. It
would be quite a lot of new code though.

Yes, I think that is the only realistic approach at providing somewhat
unambigious SQL to replicate DDL.

An issue that would have to be thought about is that there are assorted
scenarios where we generate parse trees that contain options that
cannot be generated from SQL, so there's no way to reverse-list them
into working SQL. But often those hidden options are essential to know
what the statement will really do, so I'm not sure ignoring them will be
a workable answer for replication purposes.

Dimitri's earlier patches had support for quite some commands via
deparsing and while its a noticeable amount of code it seemed to work
ok.
The last revision I could dig out is
https://github.com/dimitri/postgres/blob/d2996303c7bc6daa08cef23e3d5e07c3afb11191/src/backend/utils/adt/ddl_rewrite.c

I think some things in there would have to change (e.g. I doubt that its
good that EventTriggerData is involved at that level) but I think it
shows very roughly how it could look.

Greetings,

Andres

--
Andres Freund 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] Inconsistent format() behavior for argument-count inconsistency

* Tom Lane (t...@sss.pgh.pa.us) wrote:
 Why do we throw an error for too few arguments, but not too many?

Not sure offhand, though I could see how it might be useful.  A use-case
might be that you have a variable template string which is user defined,
where they can choose from the arguments that are passed which ones they
want displayed and how.  Otherwise, you'd have to have a bunch of
different call sites to format() depending on which options are
requested.

I'd go look at what C sprintf() does, as I feel like that's what we're
trying to emulate and see what it does.

There is also a patch that I reviewed/commented on about changing format
around a bit.  I'm guessing it needs more review/work, just havn't
gotten back around to it yet.

Thanks,

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] in-catalog Extension Scripts and Control parameters (templates?)

On 2013-01-18 12:45:02 -0500, Stephen Frost wrote:
 * Dimitri Fontaine (dimi...@2ndquadrant.fr) wrote:
  Please find attached a preliminary patch following the TEMPLATE ideas,
  and thanks in particular to Tom and Heikki for a practical design about
  how to solve that problem!
 
 Given that it's preliminary and v0 and big and whatnot, it seems like
 it should be bounced to post-9.3.  Even so, I did take a look through
 it, probably mostly because I'd really like to see this feature. :)

To be fair, the patch start its life pretty early on in the cycle and
only got really reviewed (and I think updated) later. I just got
rewritten into this form based on review.

 'Template' seems like a really broad term which might end up being
 associated with things beyond extensions, yet there are a number of
 places where you just use 'TEMPLATE', eg, ACL_KIND_TEMPLATE.  Seems like
 it might become an issue later.

I think Tom came up with that name and while several people (including
me and I think also Dim) didn't really like it nobody has come up with a
better name so far.

Greetings,

Andres Freund

-- 
 Andres Freund 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] missing rename support

* Ali Dar (ali.munir@gmail.com) wrote:
 Find attached an initial patch for ALTER RENAME RULE feature. Please
 note that it does not have any documentation yet.

Just took a quick look through this.  Seems to be alright, but why do we
allow renaming ON SELECT rules at all, given that they must be named
_RETURN?  My thinking would be to check for that case and error out if
someone tries it.

You should try to keep variables lined up:

Relationpg_rewrite_desc;
HeapTuple   ruletup;
+   Oid owningRel;

should be:

Relationpg_rewrite_desc;
HeapTuple   ruletup;
+   Oid owningRel;

I'd also strongly recommend looking through that entire function very
carefully.  Code that's been #ifdef'd out tends to rot.

Thanks,

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] in-catalog Extension Scripts and Control parameters (templates?)

* Andres Freund (and...@2ndquadrant.com) wrote:
 On 2013-01-18 12:45:02 -0500, Stephen Frost wrote:
  'Template' seems like a really broad term which might end up being
  associated with things beyond extensions, yet there are a number of
  places where you just use 'TEMPLATE', eg, ACL_KIND_TEMPLATE.  Seems like
  it might become an issue later.
 
 I think Tom came up with that name and while several people (including
 me and I think also Dim) didn't really like it nobody has come up with a
 better name so far.

'Extension Template' is fine, I was just objecting to places in the code
where it just says 'TEMPLATE'.  I imagine we might have some 'XXX
Template' at some point in the future and then we'd have confusion
between is this an *extension* template or an XXX template?.

Thanks,

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Event Triggers: adding information

Andres Freund escribió:
 On 2013-01-18 12:44:13 -0500, Tom Lane wrote:

  An issue that would have to be thought about is that there are assorted
  scenarios where we generate parse trees that contain options that
  cannot be generated from SQL, so there's no way to reverse-list them
  into working SQL.  But often those hidden options are essential to know
  what the statement will really do, so I'm not sure ignoring them will be
  a workable answer for replication purposes.
 
 Dimitri's earlier patches had support for quite some commands via
 deparsing and while its a noticeable amount of code it seemed to work
 ok.
 The last revision I could dig out is
 https://github.com/dimitri/postgres/blob/d2996303c7bc6daa08cef23e3d5e07c3afb11191/src/backend/utils/adt/ddl_rewrite.c
 
 I think some things in there would have to change (e.g. I doubt that its
 good that EventTriggerData is involved at that level) but I think it
 shows very roughly how it could look.

I looked at that code back then and didn't like it very much.  The
problem I see (as Robert does, I think) is that it raises the burden
when you change the grammar -- you now need to edit not only gram.y, but
the ddl_rewrite.c stuff to ensure your new thing can be reverse-parsed.
One idea I had was to add annotations on gram.y so that an external
(Perl?) program could generate the ddl_rewrite.c equivalent, without
forcing the developer to do it by hand.

Another problem is what Tom mentions: there are internal options that
cannot readily be turned back into SQL.  We would have to expose these
at the SQL level somehow; but to me that looks painful because we would
be offering users the option to break their systems by calling commands
that do things that should only be done internally.

-- 
Álvaro Herrerahttp://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] 9.3 Pre-proposal: Range Merge Join

2013-01-18 Thread Jeff Davis

On Fri, 2013-01-18 at 12:25 +0100, Stefan Keller wrote:
 Sounds good.
 Did you already had contact e.g. with Paul (cc'ed just in case)?
 And will this clever index also be available within all these hundreds
 of PostGIS functions?

Yes, I've brought the idea up to Paul before, but thank you.

It's not an index exactly, it's a new join algorithm that should be more
efficient for spatial joins. That being said, it's not done, so it could
be an index by the time I'm finished ;)

When it is done, it will probably need some minor work in PostGIS to
make use of it. But that work is done at the datatype level, and PostGIS
only has a couple data types, so I don't think it will be a lot of work.

Regards,
Jeff Davis



-- 
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] Event Triggers: adding information

On 2013-01-18 15:22:55 -0300, Alvaro Herrera wrote:
Andres Freund escribió:
On 2013-01-18 12:44:13 -0500, Tom Lane wrote:

I think some things in there would have to change (e.g. I doubt that its
good that EventTriggerData is involved at that level) but I think it
shows very roughly how it could look.

I looked at that code back then and didn't like it very much. The
problem I see (as Robert does, I think) is that it raises the burden
when you change the grammar -- you now need to edit not only gram.y, but
the ddl_rewrite.c stuff to ensure your new thing can be reverse-parsed.

Yea, but thats nothing new, you already need to do all that for normal
SQL. Changes that to the grammar that don't involve modifying ruleutils.c et
al. are mostly trivial enough that this doesn't really place that much of a
burden.

And I yet to hear any other proposal of how to do this?

Another problem is what Tom mentions: there are internal options that
cannot readily be turned back into SQL. We would have to expose these
at the SQL level somehow; but to me that looks painful because we would
be offering users the option to break their systems by calling commands
that do things that should only be done internally.

Hm. Which options are you two thinking of right now?

Greetings,

Andres Freund

--
Andres Freund 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] in-catalog Extension Scripts and Control parameters (templates?)

Stephen Frost sfr...@snowman.net writes:
 'Extension Template' is fine, I was just objecting to places in the code
 where it just says 'TEMPLATE'.  I imagine we might have some 'XXX
 Template' at some point in the future and then we'd have confusion
 between is this an *extension* template or an XXX template?.

We already do: see text search templates.  The code tends to call those
TSTEMPLATEs, so I'd suggest ACL_KIND_EXTTEMPLATE or some such.  I agree
with Stephen's objection to use of the bare word template.

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] missing rename support

Stephen Frost sfr...@snowman.net writes:
 * Ali Dar (ali.munir@gmail.com) wrote:
 Find attached an initial patch for ALTER RENAME RULE feature. Please
 note that it does not have any documentation yet.

 Just took a quick look through this.  Seems to be alright, but why do we
 allow renaming ON SELECT rules at all, given that they must be named
 _RETURN?  My thinking would be to check for that case and error out if
 someone tries it.

Agreed, we should exclude ON SELECT rules.

 You should try to keep variables lined up:

pgindent is probably a better answer than trying to get this right
manually.

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] Event Triggers: adding information

Alvaro Herrera alvhe...@2ndquadrant.com writes:
 Andres Freund escribió:
 Dimitri's earlier patches had support for quite some commands via
 deparsing and while its a noticeable amount of code it seemed to work
 ok.
 The last revision I could dig out is
 https://github.com/dimitri/postgres/blob/d2996303c7bc6daa08cef23e3d5e07c3afb11191/src/backend/utils/adt/ddl_rewrite.c

 I looked at that code back then and didn't like it very much.  The
 problem I see (as Robert does, I think) is that it raises the burden
 when you change the grammar -- you now need to edit not only gram.y, but
 the ddl_rewrite.c stuff to ensure your new thing can be reverse-parsed.

Well, that burden already exists for non-utility statements --- why
should utility statements get a pass?  Other than that we tend to invent
new utility syntax freely, which might be a good thing to discourage
anyhow.

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] in-catalog Extension Scripts and Control parameters (templates?)

2013-01-18 Thread Dimitri Fontaine

Tom Lane t...@sss.pgh.pa.us writes:
 We already do: see text search templates.  The code tends to call those
 TSTEMPLATEs, so I'd suggest ACL_KIND_EXTTEMPLATE or some such.  I agree
 with Stephen's objection to use of the bare word template.

Yes, me too, but I had a hard time to convince myself of using such a
wordy notation. I will adjust the patch. Is that all I have to adjust
before finishing the command set support? :)

Regards,
-- 
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support


-- 
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] in-catalog Extension Scripts and Control parameters (templates?)

* Dimitri Fontaine (dimi...@2ndquadrant.fr) wrote:
 Tom Lane t...@sss.pgh.pa.us writes:
  We already do: see text search templates.  The code tends to call those
  TSTEMPLATEs, so I'd suggest ACL_KIND_EXTTEMPLATE or some such.  I agree
  with Stephen's objection to use of the bare word template.
 
 Yes, me too, but I had a hard time to convince myself of using such a
 wordy notation. I will adjust the patch. Is that all I have to adjust
 before finishing the command set support? :)

I'm keeping a healthy distance away from *that*.. ;)

Thanks,

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] could not create directory ...: File exists

Stephen Frost sfr...@snowman.net writes:
   Patch attached.  Passes the regression tests and our internal testing
   shows that it eliminates the error we were seeing (and doesn't cause
   blocking, which is even better).

   We have a workaround in place for our build system (more-or-less
   don't do that approach), but it'd really be great if this was
   back-patched and in the next round of point releases.  Glancing
   through the branches, looks like it should apply pretty cleanly.

It looks like it will work back to 8.4; before that we didn't have
RegisterSnapshot.  The patch could be adjusted for 8.3 if anyone
is sufficiently excited about it, but personally I'm 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] foreign key locks

Alvaro Herrera wrote:
 Here's version 28 of this patch.  The only substantive change here from
 v26 is that I've made GetTupleForTrigger() use either LockTupleExclusive
 or LockTupleNoKeyExclusive, depending on whether the key columns are
 being modified by the update.  This needs to go through EvalPlanQual, so
 that function is now getting the lock mode as a parameter instead of
 hardcoded LockTupleExclusive.  (All other users of GetTupleForTrigger
 still use LockTupleExclusive, so there's no change for anybody other
 than FOR EACH ROW BEFORE UPDATE triggers).
 
 At this point, I think I've done all I wanted to do in connection with
 this patch, and I intend to commit.  I think this is a good time to get
 it committed, so that people can play with it more extensively and
 report any breakage I might have caused before we even get into beta.

While trying this out this morning I noticed a bug in the XLog code:
trying to lock the updated version of a tuple when the page that
contains the updated version requires a backup block, would cause this:

PANIC:  invalid xlog record length 0

The reason is that there is an (unknown to me) rule that there must be
some data not associated with a buffer:

/*
 * NOTE: We disallow len == 0 because it provides a useful bit of extra
 * error checking in ReadRecord.  This means that all callers of
 * XLogInsert must supply at least some not-in-a-buffer data. [...]
 */

This seems pretty strange to me.  And having the rule be spelled out
only in a comment within XLogInsert and not at its top, and not nearby
the XLogRecData struct definition either, seems pretty strange to me.
I wonder how come every PG hacker except me knows this.

For the curious, I attach an isolationtester spec file I used to
reproduce the problem (and verify the fix).

To fix the crash I needed to do this:

diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 99fced1..9762890 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -4838,7 +4838,7 @@ l4:
{
xl_heap_lock_updated xlrec;
XLogRecPtr  recptr;
-   XLogRecData rdata;
+   XLogRecData rdata[2];
Pagepage = BufferGetPage(buf);
 
xlrec.target.node = rel-rd_node;
@@ -4846,13 +4846,18 @@ l4:
xlrec.xmax = new_xmax;
xlrec.infobits_set = compute_infobits(new_infomask, 
new_infomask2);
 
-   rdata.data = (char *) xlrec;
-   rdata.len = SizeOfHeapLockUpdated;
-   rdata.buffer = buf;
-   rdata.buffer_std = true;
-   rdata.next = NULL;
+   rdata[0].data = (char *) xlrec;
+   rdata[0].len = SizeOfHeapLockUpdated;
+   rdata[0].buffer = InvalidBuffer;
+   rdata[0].next = (rdata[1]);
+
+   rdata[1].data = NULL;
+   rdata[1].len = 0;
+   rdata[1].buffer = buf;
+   rdata[1].buffer_std = true;
+   rdata[1].next = NULL;
 
-   recptr = XLogInsert(RM_HEAP2_ID, 
XLOG_HEAP2_LOCK_UPDATED, rdata);
+   recptr = XLogInsert(RM_HEAP2_ID, 
XLOG_HEAP2_LOCK_UPDATED, rdata);
 
PageSetLSN(page, recptr);
PageSetTLI(page, ThisTimeLineID);

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training  Services
setup
{
  CREATE TABLE foo (
key serial PRIMARY KEY,
value   int
  );

  INSERT INTO foo SELECT value FROM generate_series(1, 226) AS value;
}

teardown
{
  DROP TABLE foo;
}

session s1
step s1b  { BEGIN ISOLATION LEVEL REPEATABLE READ; }
step s1s  { SELECT * FROM foo LIMIT 0; }  # obtain snapshot
step s1l  { SELECT * FROM foo WHERE key = 1 FOR KEY SHARE; } # obtain lock
step s1c  { COMMIT; }

session s2
step s2b  { BEGIN; }
step s2u  { UPDATE foo SET value = 2 WHERE key = 1; }
step s2c  { COMMIT; }

session s3
step s3ck { CHECKPOINT; }

permutation s1b s1s s2b s2u s3ck s1l s1c s2c

-- 
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: Proposal for Allow postgresql.conf values to be changed via SQL [review]


Hi,

comments below.

2013-01-18 11:05 keltezéssel, Amit kapila írta:

On using mktemp, linux compilation gives below warning
warning: the use of `mktemp' is dangerous, better use `mkstemp'

So I planned to use mkstemp.


Good.


In Windows, there is an api _mktemp_s, followed by open call, behaves in a 
similar way as mkstemp available in linux.
The code is changed to use of mkstemp functionality to generate a unique 
temporary file name instead of .lock file.
Please check this part of code, I am not very comfortable with using this API.


I read the documentation of _mktemp_s() at

http://msdn.microsoft.com/en-us/library/t8ex5e91%28v=vs.80%29.aspx

The comment says it's only for C++, but I can compile your patch using
the MinGW cross-compiler under Fedora 18. So either the MSDN comment
is false, or MinGW provides this function outside C++. More research is
needed on this.

However, I am not sure whether Cygwin provides the mkstemp() call or not.
Searching... Found bugzilla reports against mkstemp on Cygwin. So, yes, it does
and your code would clash with it. In port/dirmod.c:

88888
#if defined(WIN32) || defined(__CYGWIN__)

...

/*
 *  pgmkstemp
 */
int
pgmkstemp (char *TmpFileName)
{
int err;

err = _mktemp_s(TmpFileName, strlen(TmpFileName) + 1);
if (err != 0)
return -1;

return open(TmpFileName, O_CREAT | O_RDWR | O_EXCL, S_IRUSR | S_IWUSR);
}

/* We undefined these above; now redefine for possible use below */
#define rename(from, to)pgrename(from, to)
#define unlink(path)pgunlink(path)
#define mkstemp(tempfilename)   pgmkstemp(tempfilename)
#endif   /* defined(WIN32) || defined(__CYGWIN__) */
88888

pgmkstemp() should be defined in its own block inside

#if defined(WIN32)  !defined(__CYGWIN__)
...
#endif

Same thing applies to src/include/port.h:

88888
#if defined(WIN32) || defined(__CYGWIN__)
/*
 *  Win32 doesn't have reliable rename/unlink during concurrent access.
 */
extern int  pgrename(const char *from, const char *to);
extern int  pgunlink(const char *path);
extern int pgmkstemp (char *TmpFileName);

/* Include this first so later includes don't see these defines */
#ifdef WIN32_ONLY_COMPILER
#include io.h
#endif

#define rename(from, to)pgrename(from, to)
#define unlink(path)pgunlink(path)
#define mkstemp(tempfilename)   pgmkstemp(tempfilename)
#endif   /* defined(WIN32) || defined(__CYGWIN__) */
88888


Removed the code which is used for deleting the .lock file in case of backend 
crash or during server startup.


Good.


Added a new function RemoveAutoConfTmpFiles used for deleting the temp files 
left out any during previous
postmaster session in the server startup.


Good.


In SendDir(), used sizeof() -1


Good.

Since you have these defined:

+ #define PG_AUTOCONF_DIR config_dir
+ #define PG_AUTOCONF_FILENAME postgresql.auto.conf
+ #define PG_AUTOCONF_SAMPLE_TMPFILE postgresql.auto.conf.XX
+ #define PG_AUTOCONF_TMP_FILEpostgresql.auto.conf.

then the hardcoded values can also be changed everywhere. But to kill
them in initdb.c, these #define's must be in pg_config_manual.h instead of
guc.h.

Most notably, postgresql.conf.sample contains:
#include_dir = 'auto.conf.d'
and initdb replaces it with
include_dir = 'config_dir'.
I prefer the auto.conf.d directory name. After killing all hardcoded
config_dir, changing one #define is all it takes to change it.

There are two blocks of code that Tom commented as being over-engineered:

+   /* Frame auto conf name and auto conf sample temp file name */
+   snprintf(Filename,sizeof(Filename),%s/%s, PG_AUTOCONF_DIR, 
PG_AUTOCONF_FILENAME);
+   StrNCpy(AutoConfFileName, ConfigFileName, sizeof(AutoConfFileName));
+   get_parent_directory(AutoConfFileName);
+   join_path_components(AutoConfFileName, AutoConfFileName, Filename);
+   canonicalize_path(AutoConfFileName);
+
+   snprintf(Filename,sizeof(Filename),%s/%s, PG_AUTOCONF_DIR, 
PG_AUTOCONF_SAMPLE_TMPFILE);

+   StrNCpy(AutoConfTmpFileName, ConfigFileName, 
sizeof(AutoConfTmpFileName));
+   get_parent_directory(AutoConfTmpFileName);
+   join_path_components(AutoConfTmpFileName, AutoConfTmpFileName, 
Filename);
+   canonicalize_path(AutoConfTmpFileName);

You can simply do
snprintf(AutoConfFileName, sizeof(AutoConfFileName), %s/%s/%s,
data_directory,
PG_AUTOCONF_DIR,
PG_AUTOCONF_FILENAME);
snprintf(AutoConfTmpFileName, sizeof(AutoConfTmpFileName),%s/%s/%s,
data_directory,
PG_AUTOCONF_DIR,
PG_AUTOCONF_SAMPLE_TMPFILE);

Also, mkstemp() and

Re: [HACKERS] foreign key locks

2013-01-18 Thread Simon Riggs

On 18 January 2013 20:02, Alvaro Herrera alvhe...@2ndquadrant.com wrote:
 XLOG_HEAP2_LOCK_UPDATED

Every xlog record needs to know where to put the block.

Compare with XLOG_HEAP_LOCK

xlrec.target.node = relation-rd_node;

-- 
 Simon Riggs   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: Proposal for Allow postgresql.conf values to be changed via SQL [review]

Boszormenyi Zoltan z...@cybertec.at writes:
 2013-01-18 11:05 keltezéssel, Amit kapila írta:
 On using mktemp, linux compilation gives below warning
 warning: the use of `mktemp' is dangerous, better use `mkstemp'
 
 So I planned to use mkstemp.

 Good.

On my HPUX box, the man page disapproves of both, calling them obsolete
(and this man page is old enough to vote, I believe...)

Everywhere else that we need to do something like this, we just use our
own PID to disambiguate, ie
sprintf(tempfilename, /path/to/file.%d, (int) getpid());
There is no need to deviate from that pattern or introduce portability
issues, since we can reasonably assume that no non-Postgres programs are
creating files in this directory.

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] foreign key locks

Alvaro Herrera alvhe...@2ndquadrant.com writes:
 The reason is that there is an (unknown to me) rule that there must be
 some data not associated with a buffer:

   /*
* NOTE: We disallow len == 0 because it provides a useful bit of extra
* error checking in ReadRecord.  This means that all callers of
* XLogInsert must supply at least some not-in-a-buffer data. [...]
*/

 This seems pretty strange to me.  And having the rule be spelled out
 only in a comment within XLogInsert and not at its top, and not nearby
 the XLogRecData struct definition either, seems pretty strange to me.
 I wonder how come every PG hacker except me knows this.

I doubt it ever came up before.  What use is logging only the content of
a buffer page?  Surely you'd need to know, for example, which relation
and page number it is from.

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] foreign key locks

On 2013-01-18 15:37:47 -0500, Tom Lane wrote:
 Alvaro Herrera alvhe...@2ndquadrant.com writes:
  The reason is that there is an (unknown to me) rule that there must be
  some data not associated with a buffer:
 
  /*
   * NOTE: We disallow len == 0 because it provides a useful bit of extra
   * error checking in ReadRecord.  This means that all callers of
   * XLogInsert must supply at least some not-in-a-buffer data. [...]
   */
 
  This seems pretty strange to me.  And having the rule be spelled out
  only in a comment within XLogInsert and not at its top, and not nearby
  the XLogRecData struct definition either, seems pretty strange to me.
  I wonder how come every PG hacker except me knows this.
 
 I doubt it ever came up before.  What use is logging only the content of
 a buffer page?  Surely you'd need to know, for example, which relation
 and page number it is from.

It only got to be a length of 0 because the the data got removed due to
a logged full page write. And the backup block contains the data about
which blocks it is logging in itself.
I wonder if the check shouldn't just check write_len instead, directly
below the loop that ads backup blocks.

Greetings,

Andres Freund

-- 
 Andres Freund 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: Proposal for Allow postgresql.conf values to be changed via SQL [review]


2013-01-18 21:32 keltezéssel, Tom Lane írta:

Boszormenyi Zoltan z...@cybertec.at writes:

2013-01-18 11:05 keltezéssel, Amit kapila írta:

On using mktemp, linux compilation gives below warning
warning: the use of `mktemp' is dangerous, better use `mkstemp'

So I planned to use mkstemp.

Good.

On my HPUX box, the man page disapproves of both, calling them obsolete
(and this man page is old enough to vote, I believe...)

Everywhere else that we need to do something like this, we just use our
own PID to disambiguate, ie
sprintf(tempfilename, /path/to/file.%d, (int) getpid());
There is no need to deviate from that pattern or introduce portability
issues, since we can reasonably assume that no non-Postgres programs are
creating files in this directory.


Thanks for the enlightenment, I will post a new version soon.



regards, tom lane





--
--
Zoltán Böszörményi
Cybertec Schönig  Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
 http://www.postgresql.at/



--
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] foreign key locks

Andres Freund and...@2ndquadrant.com writes:
 On 2013-01-18 15:37:47 -0500, Tom Lane wrote:
 I doubt it ever came up before.  What use is logging only the content of
 a buffer page?  Surely you'd need to know, for example, which relation
 and page number it is from.

 It only got to be a length of 0 because the the data got removed due to
 a logged full page write. And the backup block contains the data about
 which blocks it is logging in itself.

And if the full-page-image case *hadn't* been invoked, what then?  I
still don't see a very good argument for xlog records with no fixed
data.

 I wonder if the check shouldn't just check write_len instead, directly
 below the loop that ads backup blocks.

We're not changing this unless you can convince me that the read-side
error check mentioned in the comment is useless.

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: Proposal for Allow postgresql.conf values to be changed via SQL [review]


2013-01-18 21:48 keltezéssel, Boszormenyi Zoltan írta:

2013-01-18 21:32 keltezéssel, Tom Lane írta:

Boszormenyi Zoltan z...@cybertec.at writes:

2013-01-18 11:05 keltezéssel, Amit kapila írta:

On using mktemp, linux compilation gives below warning
warning: the use of `mktemp' is dangerous, better use `mkstemp'

So I planned to use mkstemp.

Good.

On my HPUX box, the man page disapproves of both, calling them obsolete
(and this man page is old enough to vote, I believe...)


Then we have to at least consider what this old {p|s}age says... ;-)



Everywhere else that we need to do something like this, we just use our
own PID to disambiguate, ie
sprintf(tempfilename, /path/to/file.%d, (int) getpid());
There is no need to deviate from that pattern or introduce portability
issues, since we can reasonably assume that no non-Postgres programs are
creating files in this directory.


Thanks for the enlightenment, I will post a new version soon.


Here it is.

Best regards,
Zoltán Böszörményi

--
--
Zoltán Böszörményi
Cybertec Schönig  Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
 http://www.postgresql.at/



set_persistent_v7.patch.gz
Description: Unix tar archive

-- 
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] foreign key locks

On 2013-01-18 16:01:18 -0500, Tom Lane wrote:
 Andres Freund and...@2ndquadrant.com writes:
  On 2013-01-18 15:37:47 -0500, Tom Lane wrote:
  I doubt it ever came up before.  What use is logging only the content of
  a buffer page?  Surely you'd need to know, for example, which relation
  and page number it is from.
 
  It only got to be a length of 0 because the the data got removed due to
  a logged full page write. And the backup block contains the data about
  which blocks it is logging in itself.
 
 And if the full-page-image case *hadn't* been invoked, what then?  I
 still don't see a very good argument for xlog records with no fixed
 data.

In that case data would have been logged?

The code in question was:

xl_heap_lock_updated xlrec;
xlrec.target.node = rel-rd_node;
...
xlrec.xmax = new_xmax;

-   rdata.data = (char *) xlrec;
-   rdata.len = SizeOfHeapLockUpdated;
-   rdata.buffer = buf;
-   rdata.buffer_std = true;
-   rdata.next = NULL;
-   recptr = XLogInsert(RM_HEAP2_ID, XLOG_HEAP2_LOCK_UPDATED, rdata);

Other wal logging code (and fklocks now as well) just put those into
two XLogRecData blocks to avoid the issue.

  I wonder if the check shouldn't just check write_len instead, directly
  below the loop that ads backup blocks.
 
 We're not changing this unless you can convince me that the read-side
 error check mentioned in the comment is useless.

Youre right, the read side check is worth quite a bit. I think I am
retracting my suggestion.
I guess the amount of extra data thats uselessly logged although never
used in in the redo functions doesn't really amass to anything
significant in comparison to the backup block data.


Greetings,

Andres Freund

-- 
 Andres Freund 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] [WIP] pg_ping utility

2013-01-18 Thread Phil Sorber

On Tue, Dec 25, 2012 at 1:47 AM, Michael Paquier
michael.paqu...@gmail.com wrote:


 On Mon, Dec 24, 2012 at 12:44 AM, Phil Sorber p...@omniti.com wrote:

 Updated patch attached.

 Thanks. I am marking this patch as ready for committer.

 --
 Michael Paquier
 http://michael.otacoo.com

Updated patch is rebased against current master and copyright year is updated.


pg_isready_bin_v10.diff
Description: Binary data


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

Re: [HACKERS] Event Triggers: adding information

On Fri, Jan 18, 2013 at 1:59 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 Alvaro Herrera alvhe...@2ndquadrant.com writes:
 Andres Freund escribió:
 Dimitri's earlier patches had support for quite some commands via
 deparsing and while its a noticeable amount of code it seemed to work
 ok.
 The last revision I could dig out is
 https://github.com/dimitri/postgres/blob/d2996303c7bc6daa08cef23e3d5e07c3afb11191/src/backend/utils/adt/ddl_rewrite.c

 I looked at that code back then and didn't like it very much.  The
 problem I see (as Robert does, I think) is that it raises the burden
 when you change the grammar -- you now need to edit not only gram.y, but
 the ddl_rewrite.c stuff to ensure your new thing can be reverse-parsed.

 Well, that burden already exists for non-utility statements --- why
 should utility statements get a pass?  Other than that we tend to invent
 new utility syntax freely, which might be a good thing to discourage
 anyhow.

My concerns are that (1) it will slow down the addition of new
features to PostgreSQL by adding yet another barrier to commit and (2)
it won't be get enough use or regression test coverage to be, or
remain, bug-free.

There may be other concerns with respect to the patch-as-proposed, but
those are my high-level concerns.

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

[HACKERS] Contrib PROGRAM problem


Hi,

I want to test my lock_timeout code under Windows and
I compiled the whole PG universe with the MinGW cross-compiler
for 64-bit under Fedora 18.

The problem contrib directories where Makefile contains
PROGRAM = ...
The executables binaries are created without the .exe suffix. E.g.:

[zozo@localhost oid2name]$ make
x86_64-w64-mingw32-gcc -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions 
--param=ssp-buffer-size=4 -Wall -Wmissing-prototypes -Wpointer-arith 
-Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute -Wformat-security 
-fno-strict-aliasing -fwrapv -fexcess-precision=standard -I../../src/interfaces/libpq -I. 
-I. -I../../src/include -I./src/include/port/win32 -DEXEC_BACKEND 
-I../../src/include/port/win32  -c -o oid2name.o oid2name.c
x86_64-w64-mingw32-gcc -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions 
--param=ssp-buffer-size=4 -Wall -Wmissing-prototypes -Wpointer-arith 
-Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute -Wformat-security 
-fno-strict-aliasing -fwrapv -fexcess-precision=standard oid2name.o -L../../src/port 
-lpgport -L../../src/interfaces/libpq -lpq -L../../src/port 
-Wl,--allow-multiple-definition-lpgport -lz -lcrypt -lwsock32 -ldl -lm  -lws2_32 
-lshfolder -o oid2name


Note the -o oid2name. Then make install of course fails, it expects
the .exe suffix:

[zozo@localhost oid2name]$ LANG=C make DESTDIR=/home/zozo/pgc93dev-win install
/usr/bin/mkdir -p 
'/home/zozo/pgc93dev-win/usr/x86_64-w64-mingw32/sys-root/mingw/bin'
/usr/bin/install -c  oid2name.exe 
'/home/zozo/pgc93dev-win/usr/x86_64-w64-mingw32/sys-root/mingw/bin'

/usr/bin/install: cannot stat 'oid2name.exe': No such file or directory
make: *** [install] Error 1

Ditto for make clean:

[zozo@localhost oid2name]$ make clean
rm -f oid2name.exe
rm -f oid2name.o

All other binaries that are under src/bin or src/backend get
the proper .exe. DLLs are OK under both contrib or src.

Best regards,
Zoltán Böszörményi

--
--
Zoltán Böszörményi
Cybertec Schönig  Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
 http://www.postgresql.at/



--
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] Contrib PROGRAM problem

Boszormenyi Zoltan wrote:


 I want to test my lock_timeout code under Windows and
 I compiled the whole PG universe with the MinGW cross-compiler
 for 64-bit under Fedora 18.
 
 The problem contrib directories where Makefile contains
 PROGRAM = ...
 The executables binaries are created without the .exe suffix. E.g.:

I think you should be able to solve this by adding the $(X) suffix to
the $(PROGRAM) rule at the bottom of src/makefiles/pgxs.mk.

-- 
Álvaro Herrerahttp://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] Event Triggers: adding information

Robert Haas robertmh...@gmail.com writes:
 On Fri, Jan 18, 2013 at 1:59 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 Well, that burden already exists for non-utility statements --- why
 should utility statements get a pass?  Other than that we tend to invent
 new utility syntax freely, which might be a good thing to discourage
 anyhow.

 My concerns are that (1) it will slow down the addition of new
 features to PostgreSQL by adding yet another barrier to commit and (2)
 it won't be get enough use or regression test coverage to be, or
 remain, bug-free.

Meh.  The barriers to inventing new statements are already mighty tall.
As for (2), I agree there's risk of bugs, but what alternative have you
got that is likely to be less bug-prone?  At least a reverse-list
capability could be tested standalone without having to set up a logical
replication configuration.

This should not be interpreted as saying I'm gung-ho to do this, mind
you.  I'm just saying that if our intention is to support logical
replication of all DDL operations, none of the alternatives look cheap.

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] Contrib PROGRAM problem


2013-01-18 22:52 keltezéssel, Alvaro Herrera írta:

Boszormenyi Zoltan wrote:



I want to test my lock_timeout code under Windows and
I compiled the whole PG universe with the MinGW cross-compiler
for 64-bit under Fedora 18.

The problem contrib directories where Makefile contains
 PROGRAM = ...
The executables binaries are created without the .exe suffix. E.g.:

I think you should be able to solve this by adding the $(X) suffix to
the $(PROGRAM) rule at the bottom of src/makefiles/pgxs.mk.



Do you mean the attached patch? It indeed fixes the build.

Best regards,
Zoltán Böszörményi

--
--
Zoltán Böszörményi
Cybertec Schönig  Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
 http://www.postgresql.at/

--- src/makefiles/pgxs.mk.orig	2013-01-18 23:10:57.808370762 +0100
+++ src/makefiles/pgxs.mk	2013-01-18 23:11:22.741554459 +0100
@@ -296,5 +296,5 @@
 
 ifdef PROGRAM
 $(PROGRAM): $(OBJS)
-	$(CC) $(CFLAGS) $(OBJS) $(PG_LIBS) $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o $@
+	$(CC) $(CFLAGS) $(OBJS) $(PG_LIBS) $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o $@$(X)
 endif

-- 
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] Event Triggers: adding information

On Fri, Jan 18, 2013 at 5:12 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 Robert Haas robertmh...@gmail.com writes:
 On Fri, Jan 18, 2013 at 1:59 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 Well, that burden already exists for non-utility statements --- why
 should utility statements get a pass?  Other than that we tend to invent
 new utility syntax freely, which might be a good thing to discourage
 anyhow.

 My concerns are that (1) it will slow down the addition of new
 features to PostgreSQL by adding yet another barrier to commit and (2)
 it won't be get enough use or regression test coverage to be, or
 remain, bug-free.

 Meh.  The barriers to inventing new statements are already mighty tall.
 As for (2), I agree there's risk of bugs, but what alternative have you
 got that is likely to be less bug-prone?  At least a reverse-list
 capability could be tested standalone without having to set up a logical
 replication configuration.

 This should not be interpreted as saying I'm gung-ho to do this, mind
 you.  I'm just saying that if our intention is to support logical
 replication of all DDL operations, none of the alternatives look cheap.

Well, we agree on that, anyway.  :-)

I honestly don't know what to do about this.  I think you, Alvaro, and
I all have roughly the same opinion of this, which is that it doesn't
sound fun, but there's probably nothing better.  So, what do we do
when a really cool potential feature (logical replication of DDL)
butts up against an expensive future maintenance requirement?  I'm not
even sure what the criteria should be for making a decision on whether
it's worth it.

-- 
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] Contrib PROGRAM problem

2013-01-18 Thread Andrew Dunstan



On 01/18/2013 05:19 PM, Boszormenyi Zoltan wrote:

2013-01-18 22:52 keltezéssel, Alvaro Herrera írta:

Boszormenyi Zoltan wrote:



I want to test my lock_timeout code under Windows and
I compiled the whole PG universe with the MinGW cross-compiler
for 64-bit under Fedora 18.

The problem contrib directories where Makefile contains
 PROGRAM = ...
The executables binaries are created without the .exe suffix. E.g.:

I think you should be able to solve this by adding the $(X) suffix to
the $(PROGRAM) rule at the bottom of src/makefiles/pgxs.mk.



Do you mean the attached patch? It indeed fixes the build.






  ifdef PROGRAM
  $(PROGRAM): $(OBJS)
-   $(CC) $(CFLAGS) $(OBJS) $(PG_LIBS) $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o 
$@
+   $(CC) $(CFLAGS) $(OBJS) $(PG_LIBS) $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o 
$@$(X)
  endif




Wouldn't it be better to make the rule be for $(PROGRAM)$(X) and adjust 
the dependency for all in the same manner? Otherwise make will rebuild 
it whether or not it's needed, won't it?



cheers

andrew




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

[HACKERS] Strange Windows problem, lock_timeout test request


Hi,

using the MinGW cross-compiled PostgreSQL binaries from Fedora 18,
I get the following error for both 32 and 64-bit compiled executables.
listen_addresses = '*' and trust authentication was set for both.
The firewall was disabled for the tests and the server logs incomplete startup 
packet.
The 64-bit version was compiled with my lock_timeout patch, the 32-bit
was without.

64-bit, connect to localhost:

C:\Users\Ákos\Desktop\PG93bin\psql
psql: could not connect to server: Operation would block (0x2733/10035)
Is the server running on host localhost (::1) and accepting
TCP/IP connections on port 5432?
could not connect to server: Operation would block (0x2733/10035)
Is the server running on host localhost (127.0.0.1) and accepting
TCP/IP connections on port 5432?

64-bit, connect to own IP:

C:\Users\Ákos\Desktop\PG93bin\psql -h 192.168.1.4
psql: could not connect to server: Operation would block (0x2733/10035)
Is the server running on host 192.168.1.4 and accepting
TCP/IP connections on port 5432?

32-bit, connect to own IP:

C:\Users\Ákos\Desktop\PG93-32bin\psql -h 192.168.1.4
psql: could not connect to server: Operation would block (0x2733/10035)
Is the server running on host 192.168.1.4 and accepting
TCP/IP connections on port 5432?

Does it ring a bell for someone?

Unfortunately, I won't have time to do anything with my lock_timeout patch
for about 3 weeks. Does anyone have a little spare time to test it on Windows?
The patch is here, it still applies to HEAD without rejects or fuzz:
http://www.postgresql.org/message-id/506c0854.7090...@cybertec.at

Thanks in advance,
Zoltán Böszörményi

--
--
Zoltán Böszörményi
Cybertec Schönig  Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
 http://www.postgresql.at/



--
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] Contrib PROGRAM problem


2013-01-18 23:37 keltezéssel, Andrew Dunstan írta:


On 01/18/2013 05:19 PM, Boszormenyi Zoltan wrote:

2013-01-18 22:52 keltezéssel, Alvaro Herrera írta:

Boszormenyi Zoltan wrote:



I want to test my lock_timeout code under Windows and
I compiled the whole PG universe with the MinGW cross-compiler
for 64-bit under Fedora 18.

The problem contrib directories where Makefile contains
 PROGRAM = ...
The executables binaries are created without the .exe suffix. E.g.:

I think you should be able to solve this by adding the $(X) suffix to
the $(PROGRAM) rule at the bottom of src/makefiles/pgxs.mk.



Do you mean the attached patch? It indeed fixes the build.






  ifdef PROGRAM
  $(PROGRAM): $(OBJS)
-$(CC) $(CFLAGS) $(OBJS) $(PG_LIBS) $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o $@
+$(CC) $(CFLAGS) $(OBJS) $(PG_LIBS) $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o 
$@$(X)
  endif




Wouldn't it be better to make the rule be for $(PROGRAM)$(X) and adjust the dependency 
for all in the same manner? Otherwise make will rebuild it whether or not it's needed, 
won't it?


With this in place:

all: $(PROGRAM)$(X) $(DATA_built) $(SCRIPTS_built) $(addsuffix $(DLSUFFIX), $(MODULES)) 
$(addsuffix .control, $(EXTENSION))


[zozo@localhost contrib]$ make
make -C adminpack all
make[1]: Entering directory 
`/home/zozo/crosscolumn/lock-timeout/12/postgresql.a/contrib/adminpack'

make[1]: *** No rule to make target `.exe', needed by `all'.  Stop.
make[1]: Leaving directory 
`/home/zozo/crosscolumn/lock-timeout/12/postgresql.a/contrib/adminpack'

make: *** [all-adminpack-recurse] Error 2

It's not a good idea it seems.




cheers

andrew







--
--
Zoltán Böszörményi
Cybertec Schönig  Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
 http://www.postgresql.at/



--
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] could not create directory ...: File exists