Re: [PATCHES] still alive?

2008-10-01 Thread Alvaro Herrera
Bruce Momjian wrote:
 
 Marc, care to do the honors?
 

Note:

1. there are several lists to kill, not just pgsql-patches.  The
database says:

 pgsql-chat
 pgsql-benchmarks
 pgsql-hackers-win32
 pgsql-hackers-pitr
 pgsql-cygwin
 pgsql-ports

2. The archives must, obviously, survive the kill, and still be
fetchable via rsync to the archives server.

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

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


Re: [PATCHES] Solve a problem of LC_TIME of windows.

2008-09-24 Thread Alvaro Herrera
Magnus Hagander wrote:

 Also, the patch needs error checking. strftime() can fail, and the
 multibyte conversion functions can certainly fail. That will need to be
 added.

What about this line?

#define STRLEN_MAX 255

Are we really sure the strftime format cannot be longer than that?

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

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


Re: [PATCHES] still alive?

2008-09-17 Thread Alvaro Herrera
Bernd Helmle wrote:
 --On Donnerstag, September 11, 2008 15:39:01 +0300 Peter Eisentraut  
 [EMAIL PROTECTED] wrote:


 Hmm, let's try this:

 Anyone who thinks the patches list should remain as separate from
 hackers, shout now (with rationale)!

 Seems i've missed something, what's then supposed to hold patches now?

pgsql-hackers

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

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


Re: [PATCHES] [HACKERS] Infrastructure changes for recovery

2008-09-12 Thread Alvaro Herrera
Simon Riggs wrote:

 --- 5716,5725 
   CheckpointStats.ckpt_sync_end_t,
   sync_secs, sync_usecs);
   
 ! elog(LOG, %s complete: wrote %d buffers (%.1f%%); 
%d transaction log file(s) added, %d removed, %d recycled; 
write=%ld.%03d s, sync=%ld.%03d s, total=%ld.%03d s,
 +  (checkpoint ?   checkpoint : restartpoint),
CheckpointStats.ckpt_bufs_written,
(double) CheckpointStats.ckpt_bufs_written * 100 / NBuffers,
CheckpointStats.ckpt_segs_added,

Very minor nit: this really needs a rework.  It is relatively OK in the
previous code, but it was already stuffing too much in a single message.
Maybe

ereport(LOG,
(errmsg(checkpoint ? checkpoint complete : restartpoint complete),
errdetail(Wrote %d buffers (%.1f%%); 
%d transaction log file(s) added, %d removed, %d recycled; 
write=%ld.%03d s, sync=%ld.%03d s, total=%ld.%03d s.,
...
)))

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

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


Re: [PATCHES] [HACKERS] Infrastructure changes for recovery

2008-09-12 Thread Alvaro Herrera
Simon Riggs wrote:
 
 On Fri, 2008-09-12 at 14:14 -0400, Alvaro Herrera wrote:
  Simon Riggs wrote:
  
   --- 5716,5725 
 
   CheckpointStats.ckpt_sync_end_t,
 sync_secs, 
   sync_usecs);
 
   ! elog(LOG, %s complete: wrote %d buffers (%.1f%%); 
  %d transaction log file(s) added, %d removed, %d 
   recycled; 
  write=%ld.%03d s, sync=%ld.%03d s, total=%ld.%03d s,
   +  (checkpoint ?   checkpoint : restartpoint),
  CheckpointStats.ckpt_bufs_written,
  (double) CheckpointStats.ckpt_bufs_written * 100 / 
   NBuffers,
  CheckpointStats.ckpt_segs_added,
  
  Very minor nit: this really needs a rework.  
 
 All I changed was the word restartpoint... its otherwise identical to
 existing message. I'd rather not change that.

The new message is not translatable, the original was.

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

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


Re: [PATCHES] libpq events patch (with sgml docs)

2008-09-05 Thread Alvaro Herrera
Andrew Chernow escribió:

 ! 
 printfPQExpBuffer(conn-errorMessage,
 ! 
 libpq_gettext(PGEventProc \%s\ failed during PGEVT_CONNRESET event\n),
 ! conn-events[i].name);
 ! else
 ! 
 printfPQExpBuffer(conn-errorMessage,
 ! 
 libpq_gettext(PGEventProc \addr:%p\ failed during PGEVT_CONNRESET 
 event\n),
 ! conn-events[i].proc);
 ! break;

Please don't do this.  It creates extra unnecessary work for
translators.  Better create a local var, assign either name or
addr:value to it, and then use that in the message.

(For the record, I'd prefer that the name is made mandatory.)

Oh, BTW: don't post to pgsql-patches.  It's deprecated.  Use
pgsql-hackers instead.

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

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


Re: [PATCHES] libpq events patch

2008-09-03 Thread Alvaro Herrera
Andrew Chernow wrote:
 This is an updated version pf the libpqevents patch.  See

 http://archives.postgresql.org/pgsql-hackers/2008-09/msg00153.php

 for details.  The only change I didn't make yet is the event 'name'.  I  
 have put it in and taken it out twice now, so a firm 'put it in there'  
 would be appreciated.

You didn't merge the other changes I did to your patch.  Please use that
one as starting point, or merge them into your version.  They were
minor, sure, but I went some lengths to make them, which means the code
kinda has my signoff that way.  Please don't waste that work.

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

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


Re: [PATCHES] psql command setting

2008-08-29 Thread Alvaro Herrera
Simon Riggs wrote:

 At the very least we should document the two toggles that are already
 settable, with the attached patch.

Applied.

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

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


Re: [PATCHES] [HACKERS] Hint Bits and Write I/O

2008-08-01 Thread Alvaro Herrera
Simon Riggs wrote:

 The first half is actually quite large, but that makes it even more
 sensible to commit this part now.
 
 The enclosed patch introduces the machinery by which we might later
 optimise hint bit setting. It differentiates between hint bit setting
 and block dirtying, when the distinction can safely be made. It acts
 safely during VACUUM and correctly during checkpoint. In all other
 respects it emulates current behaviour.

I think it makes sense to commit this patch now, per previous
discussions on which we have agreed to make incremental changes.  I
think we should just get rid of the bogus changes Pavan identified.

I'm just wondering if the change of usage_count from 16 to 8 bits was
discussed and agreed?

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

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


Re: [PATCHES] Is autovacuum doing a wraparound-avoiding VACUUM?

2008-07-21 Thread Alvaro Herrera
Simon Riggs wrote:

 On Fri, 2008-07-18 at 01:44 -0400, Tom Lane wrote:
  I agree, this is important for visibility into what's happening.
  The string isn't getting translated so I don't see any big downside
  to applying the patch in back branches.
 
 Patches for 8.3 and CVS HEAD.

Applied, thanks.

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

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


Re: [PATCHES] Is autovacuum doing a wraparound-avoiding VACUUM?

2008-07-17 Thread Alvaro Herrera
Simon Riggs wrote:
 
 Is autovacuum doing a wraparound-avoiding VACUUM?
 Currently, no easy way to tell.
 
 Patch to change message of autovac in pg_stat_activity when we are
 performing an anti-wraparound VACUUM.

I just obsoleted this patch.  The new patch should be easier to do
though -- just a one line change I think.

I don't like your wording though; it feels too verbose (and you're
losing the ANALYZE in case it's doing both things).  How about 

snprintf(activity, MAX_AUTOVAC_ACTIV_LEN,
autovacuum: VACUUM%s%s, vac
tab-at_doanalyze ?  ANALYZE : ,
tab-at_wraparound ?  (wraparound) : );

You're not proposing it for 8.3 right?

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

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


Re: [PATCHES] Extending grant insert on tables to sequences

2008-07-08 Thread Alvaro Herrera
Jaime Casanova escribió:
 On Thu, May 22, 2008 at 1:18 PM, Jaime Casanova [EMAIL PROTECTED] wrote:
  Hi,
 
  The idea of this patch is to avoid the need to make explicit grants on
  sequences owned by tables.
 
 I've noted that the patch i attached is an older version that doesn't
 compile because of a typo...
 Re-attaching right patch and fix documentation to indicate the new 
 behaviour...

I had a look at this patch and it looks good.  The only thing that's not
clear to me is whether we have agreed we want this to be the default
behavior?

A quibble:

 + foreach(cell, istmt.objects)
 + {
 + [...]
 + 
 + istmt_seq.objects = getOwnedSequences(lfirst_oid(cell));
 + if (istmt_seq.objects != NIL)
 + {
 + if (istmt.privileges  (ACL_INSERT)) 
 + istmt_seq.privileges |= ACL_USAGE;
 + else if (istmt.privileges  (ACL_UPDATE)) 
 + istmt_seq.privileges |= ACL_UPDATE;
 + else if (istmt.privileges  (ACL_SELECT)) 
 + istmt_seq.privileges |= ACL_SELECT;
 + 
 + ExecGrantStmt_oids(istmt_seq);
 + }

Wouldn't it be clearer to build a list with all the sequences owned by
the tables in istmt.objects, and then call ExecGrantStmt_oids() a single
time with the big list?

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

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


Re: [PATCHES] [HACKERS] WITH RECURSIVE updated to CVS TIP

2008-07-08 Thread Alvaro Herrera
David Fetter wrote:
 On Tue, Jul 08, 2008 at 06:01:05PM +0900, Tatsuo Ishii wrote:
  Here is the patches he made against CVS HEAD (as of today).
 
 The git repository should now match this :)
 
 http://git.postgresql.org/?p=~davidfetter/postgresql/.git;a=summary
 
 Apparently, it's easiest to clone via the following URL:
 
 http://git.postgresql.org/git/~davidfetter/postgresql/.git
 
 Is there some git repository I can pull from to make this a little
 less manual?

In fact, I fail to see the point of you providing the repo if the
upstream guys are apparently not using it ...

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

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


Re: [PATCHES] GIN improvements

2008-07-07 Thread Alvaro Herrera
Teodor Sigaev wrote:
 Sync with current CVS HEAD and post in hackers- too because patches- 
 close to the closing.

 http://www.sigaev.ru/misc/multicolumn_gin-0.3.gz

I looked this over and it looks good in general.  I was only wondering
about for single-column indexes -- we're storing attribute numbers too,
right?  Would it be too difficult to strip them out in that case?

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

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


Re: [PATCHES] GIN improvements

2008-07-07 Thread Alvaro Herrera
Teodor Sigaev wrote:
 I looked this over and it looks good in general.  I was only wondering
 about for single-column indexes -- we're storing attribute numbers too,
 right?
 No, GinState-oneCol field signals to GinFormTuple and  
 gin_index_getattr/gintuple_get_attrnum about actual storage.

 Single column index is binary compatible with current index :)

Ah, neat!

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

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


Re: [PATCHES] Fix pg_ctl restart bug

2008-06-26 Thread Alvaro Herrera
Bruce Momjian wrote:
 Tom Lane wrote:
  Bruce Momjian [EMAIL PROTECTED] writes:
   However, as of 2004-10-15, this has not worked.  :-(  The attached patch
   is the one that caused the bug --- on non-Unix systems, SYSTEMQUOTE is
   , meaning zero-length string.  I should have seen the bug when I
   applied the contributed patch in 2004.
  
  So, shouldn't this fix be back-patched?
 
 Well, no one has actually complained about the breakage, and it has been
 a few years.  Also there is always the risk of a new bug being
 introduced, so I am unsure.

Why do we need someone to complain?  We know the bug is there.  Has the
code changed a lot in that area?

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

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


Re: [PATCHES] Fix pg_ctl restart bug

2008-06-26 Thread Alvaro Herrera
Bruce Momjian wrote:
 Alvaro Herrera wrote:

  Why do we need someone to complain?  We know the bug is there.  Has the
  code changed a lot in that area?
 
 Do we have the policy of backpatching every fix?  I thought it was only
 the major bugs we fixed in back branches.  If someone wants to backpatch
 it, feel free to do so.

I think the policy is we fix the bugs in supported releases.  If you
start making exceptions, it becomes needlessly complex.

I've always assumed that I'm supposed to backpatch the bugs I fix in
HEAD, however far is reasonable.

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

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


Re: [PATCHES] [HACKERS] Hint Bits and Write I/O

2008-06-18 Thread Alvaro Herrera
Simon Riggs wrote:

 When running a VACUUM command we always dirty the block when setting
 hint bits, for a number of reasons:
 * VACUUM FULL expects all hint bits to be set prior to moving tuples
 * Setting all hint bits allows us to truncate the clog
 * it forces the VACUUM to write out its own dirty buffers, which is OK,
 since it is a background process.
 
 Other commands call HeapTupleSatisfiesVacuum(), yet these tasks can be
 more flexible with hint bit setting. These include ANALYZE, CREATE
 INDEX, CLUSTER, HOT pruning and index scan marking deleted tuples (with
 changes in all index AMs). This means we have to differentiate between
 VACUUM and other callers of HeapTupleSatisfiesVacuum().
 
 So the patch changes the APIs of HeapTupleSatisfiesVacuum(),
 SetBufferCommitInfoNeedsSave() and SetHintBits() with changes to 13 AM
 and command files. There are many changes in tqual.c, which seems the
 right way because SetHintBits() is inlined. These make the patch fairly
 large, though most of it is simple changes.

If only VACUUM is going to set flexible to off, maybe it's better to
leave the APIs as they are and have a global that's set by VACUUM only
(and reset in a PG_CATCH block).

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

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


Re: [PATCHES] Simplify formatting.c

2008-06-14 Thread Alvaro Herrera
Bruce Momjian wrote:

 I moved str_initcap() over into oracle_compat.c and then had initcap()
 convert to/from TEXT to call it.  The code is a little weird because
 str_initcap() needs to convert to text to use texttowcs(), so in
 multibyte encodings initcap converts the string to text, then to char,
 then to text to call texttowcs().  I didn't see a cleaner way to do
 this.

Why not use wchar2char?  It seems there's room for extra cleanup here.

Also, the prototype of str_initcap in builtins.h looks out of place.

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

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


Re: [PATCHES] relscan.h split

2008-06-14 Thread Alvaro Herrera
Tom Lane wrote:

 Perhaps a better idea would be to put the opaque-pointer typedefs into
 heapam.h and genam.h respectively, and then see where you could remove
 inclusions of relscan.h.

Hmm, this seems to be closely equivalent.  Patch attached.  I also moved
SysScanDescData from genam.h to relscan.h.

 Also, it seemed like some of those .c files had no business poking into
 the scan structs anyway; particularly contrib.  Did you check whether
 the inclusions could be avoided?

Not really, unless we were to provide something a routine that returns
the current block of a scan.  There are a few occurrences of this:

/* must hold a buffer lock to call HeapTupleSatisfiesUpdate */
LockBuffer(scan-rs_cbuf, BUFFER_LOCK_SHARE);

which of course need the definition.  Maybe providing it is not a bad
idea, because that kind of coding is used in the backend too.

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Index: contrib/pgrowlocks/pgrowlocks.c
===
RCS file: /home/alvherre/cvs/pgsql/contrib/pgrowlocks/pgrowlocks.c,v
retrieving revision 1.10
diff -c -p -r1.10 pgrowlocks.c
*** contrib/pgrowlocks/pgrowlocks.c	12 May 2008 00:00:43 -	1.10
--- contrib/pgrowlocks/pgrowlocks.c	14 Jun 2008 23:16:20 -
***
*** 26,31 
--- 26,32 
  
  #include access/heapam.h
  #include access/multixact.h
+ #include access/relscan.h
  #include access/xact.h
  #include catalog/namespace.h
  #include funcapi.h
Index: contrib/pgstattuple/pgstattuple.c
===
RCS file: /home/alvherre/cvs/pgsql/contrib/pgstattuple/pgstattuple.c,v
retrieving revision 1.35
diff -c -p -r1.35 pgstattuple.c
*** contrib/pgstattuple/pgstattuple.c	16 May 2008 17:31:17 -	1.35
--- contrib/pgstattuple/pgstattuple.c	14 Jun 2008 23:20:41 -
***
*** 29,34 
--- 29,35 
  #include access/heapam.h
  #include access/htup.h
  #include access/nbtree.h
+ #include access/relscan.h
  #include catalog/namespace.h
  #include funcapi.h
  #include miscadmin.h
*** pgstat_heap(Relation rel, FunctionCallIn
*** 262,268 
  	/* Disable syncscan because we assume we scan from block zero upwards */
  	scan = heap_beginscan_strat(rel, SnapshotAny, 0, NULL, true, false);
  
! 	nblocks = scan-rs_nblocks; /* # blocks to be scanned */
  
  	/* scan the relation */
  	while ((tuple = heap_getnext(scan, ForwardScanDirection)) != NULL)
--- 263,269 
  	/* Disable syncscan because we assume we scan from block zero upwards */
  	scan = heap_beginscan_strat(rel, SnapshotAny, 0, NULL, true, false);
  
! 	nblocks = RelationGetNumberOfBlocks(rel); /* # blocks to be scanned */
  
  	/* scan the relation */
  	while ((tuple = heap_getnext(scan, ForwardScanDirection)) != NULL)
Index: src/backend/access/gin/ginget.c
===
RCS file: /home/alvherre/cvs/pgsql/src/backend/access/gin/ginget.c,v
retrieving revision 1.16
diff -c -p -r1.16 ginget.c
*** src/backend/access/gin/ginget.c	16 May 2008 16:31:01 -	1.16
--- src/backend/access/gin/ginget.c	14 Jun 2008 23:05:11 -
***
*** 15,20 
--- 15,21 
  #include postgres.h
  
  #include access/gin.h
+ #include access/relscan.h
  #include catalog/index.h
  #include miscadmin.h
  #include storage/bufmgr.h
Index: src/backend/access/gin/ginscan.c
===
RCS file: /home/alvherre/cvs/pgsql/src/backend/access/gin/ginscan.c,v
retrieving revision 1.14
diff -c -p -r1.14 ginscan.c
*** src/backend/access/gin/ginscan.c	16 May 2008 16:31:01 -	1.14
--- src/backend/access/gin/ginscan.c	14 Jun 2008 23:05:01 -
***
*** 14,21 
  
  #include postgres.h
  
- #include access/genam.h
  #include access/gin.h
  #include pgstat.h
  #include storage/bufmgr.h
  #include utils/memutils.h
--- 14,21 
  
  #include postgres.h
  
  #include access/gin.h
+ #include access/relscan.h
  #include pgstat.h
  #include storage/bufmgr.h
  #include utils/memutils.h
Index: src/backend/access/gist/gistget.c
===
RCS file: /home/alvherre/cvs/pgsql/src/backend/access/gist/gistget.c,v
retrieving revision 1.73
diff -c -p -r1.73 gistget.c
*** src/backend/access/gist/gistget.c	12 May 2008 00:00:44 -	1.73
--- src/backend/access/gist/gistget.c	14 Jun 2008 22:53:35 -
***
*** 15,20 
--- 15,21 
  #include postgres.h
  
  #include access/gist_private.h
+ #include access/relscan.h
  #include executor/execdebug.h
  #include miscadmin.h
  #include pgstat.h
Index: src/backend/access/gist/gistscan.c
===
RCS file: /home/alvherre/cvs/pgsql/src/backend/access/gist/gistscan.c,v
retrieving revision 1.69
diff

[PATCHES] relscan.h split

2008-06-12 Thread Alvaro Herrera
Hi,

relscan.h is very widely used -- in particular it is included by some
headers that want the IndexScanDesc and HeapScanDesc definitions in
prototypes.  However, most of the time they are just passing the struct
through; they don't need to see the actual Heap/IndexScanDescData
definitions.

I propose the following patch which moves the struct definitions to a
separate new header relscan_internal.h.  Files that actually need the
definitions can include the new header.  They are not that many -- I
count 22 inclusions, all of them in .c files.  Headers only include the
.h file, which has the benefit that since it is a lean file, it needn't
include all the other headers needed by the struct declaration.

Zdenek says that this patch changes the number of times certain headers
are opened (data gathered using dtrace):

new old diff

src/include/access/skey.h   347 465 -118

src/include/utils/rel.h 851 921 -70 

src/include/access/relscan.h340 360 -20 


So it doesn't have a tremendous impact, but it does have some.

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Index: contrib/pgrowlocks/pgrowlocks.c
===
RCS file: /home/alvherre/Code/cvs/pgsql/contrib/pgrowlocks/pgrowlocks.c,v
retrieving revision 1.10
diff -c -p -r1.10 pgrowlocks.c
*** contrib/pgrowlocks/pgrowlocks.c	12 May 2008 00:00:43 -	1.10
--- contrib/pgrowlocks/pgrowlocks.c	9 Jun 2008 01:49:31 -
***
*** 26,31 
--- 26,32 
  
  #include access/heapam.h
  #include access/multixact.h
+ #include access/relscan_internal.h
  #include access/xact.h
  #include catalog/namespace.h
  #include funcapi.h
Index: contrib/pgstattuple/pgstattuple.c
===
RCS file: /home/alvherre/Code/cvs/pgsql/contrib/pgstattuple/pgstattuple.c,v
retrieving revision 1.35
diff -c -p -r1.35 pgstattuple.c
*** contrib/pgstattuple/pgstattuple.c	16 May 2008 17:31:17 -	1.35
--- contrib/pgstattuple/pgstattuple.c	9 Jun 2008 01:49:44 -
***
*** 29,34 
--- 29,35 
  #include access/heapam.h
  #include access/htup.h
  #include access/nbtree.h
+ #include access/relscan_internal.h
  #include catalog/namespace.h
  #include funcapi.h
  #include miscadmin.h
Index: src/backend/access/gin/ginget.c
===
RCS file: /home/alvherre/Code/cvs/pgsql/src/backend/access/gin/ginget.c,v
retrieving revision 1.16
diff -c -p -r1.16 ginget.c
*** src/backend/access/gin/ginget.c	16 May 2008 16:31:01 -	1.16
--- src/backend/access/gin/ginget.c	8 Jun 2008 23:58:24 -
***
*** 15,20 
--- 15,21 
  #include postgres.h
  
  #include access/gin.h
+ #include access/relscan_internal.h
  #include catalog/index.h
  #include miscadmin.h
  #include storage/bufmgr.h
Index: src/backend/access/gin/ginscan.c
===
RCS file: /home/alvherre/Code/cvs/pgsql/src/backend/access/gin/ginscan.c,v
retrieving revision 1.14
diff -c -p -r1.14 ginscan.c
*** src/backend/access/gin/ginscan.c	16 May 2008 16:31:01 -	1.14
--- src/backend/access/gin/ginscan.c	8 Jun 2008 23:59:31 -
***
*** 16,21 
--- 16,22 
  
  #include access/genam.h
  #include access/gin.h
+ #include access/relscan_internal.h
  #include pgstat.h
  #include storage/bufmgr.h
  #include utils/memutils.h
Index: src/backend/access/gist/gistget.c
===
RCS file: /home/alvherre/Code/cvs/pgsql/src/backend/access/gist/gistget.c,v
retrieving revision 1.73
diff -c -p -r1.73 gistget.c
*** src/backend/access/gist/gistget.c	12 May 2008 00:00:44 -	1.73
--- src/backend/access/gist/gistget.c	8 Jun 2008 23:55:58 -
***
*** 15,20 
--- 15,21 
  #include postgres.h
  
  #include access/gist_private.h
+ #include access/relscan_internal.h
  #include executor/execdebug.h
  #include miscadmin.h
  #include pgstat.h
Index: src/backend/access/gist/gistscan.c
===
RCS file: /home/alvherre/Code/cvs/pgsql/src/backend/access/gist/gistscan.c,v
retrieving revision 1.69
diff -c -p -r1.69 gistscan.c
*** src/backend/access/gist/gistscan.c	12 May 2008 00:00:44 -	1.69
--- src/backend/access/gist/gistscan.c	8 Jun 2008 23:56:23 -
***
*** 17,22 
--- 17,23 
  #include access/genam.h
  #include access/gist_private.h
  #include access/gistscan.h
+ #include access

Re: [PATCHES] Tentative patch for making DROP put dependency info in DETAIL

2008-06-12 Thread Alvaro Herrera
Alex Hunsaker escribió:

 Ok I'm obviously missing something important... Why not Just make the
 various Remove* functions take a list?
 
 I'm not proposing this patch for actual submission, more of a would this work?
 If I'm not missing something glaring obvious Ill go ahead and make the
 rest of the Remove things behave the same way

I don't think there's anything wrong with that in principle.  However,
does your patch actually work?  The changes in expected/ is unexpected,
I think.

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

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


Re: [PATCHES] Tentative patch for making DROP put dependency info in DETAIL

2008-06-11 Thread Alvaro Herrera
Tom Lane wrote:

 After looking again, I think that this is not technically very
 difficult, but coming up with something that looks tasteful to everyone
 might be tricky.  In particular I didn't see a nice way to do it without
 using struct ObjectAddress in a bunch of header files that don't
 currently include dependency.h.  A possible response to that is to move
 ObjectAddress into postgres.h, but that seems a bit ugly too.

Ugh.  I thought about having a new header, but that seems overkill.

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

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


Re: [PATCHES] Tentative patch for making DROP put dependency info in DETAIL

2008-06-10 Thread Alvaro Herrera
Tom Lane wrote:

 One particular case of interest is in truncate.out, where the
 table-at-a-time implementation of DROP TABLE is clearly exposed
 by the fact that you get multiple NOTICEs.  I wonder if it would
 be worth refactoring the code so that a multiple-object DROP is
 implemented via performMultipleDeletions().  This would have more
 than just cosmetic advantages: it would no longer matter what
 order you listed the tables in.  But the refactoring required looks
 bigger and more tedious than I want to tackle right now.

Hmm, this is a bit ugly.  I'd vote for doing the refactoring.  However,
I'd say you should commit the patch you currently have and let one of
the younger hackers fix that problem -- it looks like an good beginner
project.

 I also want to note that we've historically had the code report
 auto-cascade drops as DEBUG2 messages.  I tried to merge those reports
 into the main one but it was a complete mess :-( because the client and
 server-log messages might have different numbers of entries depending on
 their log-level settings.  Almost the first case I tried favored me with
   NOTICE: drop cascades to 0 other object(s)
   DETAIL:
 reported to the client (with the server log of course saying something
 different).  So I gave up and left that behavior separate.

Huh, annoying.  Agreed with leaving that alone.

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

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


Re: [PATCHES] SQL: table function support

2008-06-10 Thread Alvaro Herrera
Tom Lane escribió:
 Alvaro Herrera [EMAIL PROTECTED] writes:
  Tom Lane escribi�:
  (It's also worth asking where the import is coming from.  Who implements
  the spec syntax anyway?  DB2 maybe, but when was the last time we heard
  from anyone trying to migrate from DB2 to PG?)
 
  Sourceforge?
 
 They gave up on us years ago :-(

Actually they migrated from 7.2 to DB2 because IBM paid for them to do
it (and also because they were tripping on some deficiency at the time);
but after that contract expired, they migrated back from DB2 to a newer
Postgres.  Obviously there wasn't much money invested in making big
press splashes about it, though.

I can't tell if they were using table function support though :-)

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

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


Re: [PATCHES] SQL: table function support

2008-06-09 Thread Alvaro Herrera
Tom Lane escribió:

 (It's also worth asking where the import is coming from.  Who implements
 the spec syntax anyway?  DB2 maybe, but when was the last time we heard
 from anyone trying to migrate from DB2 to PG?)

Sourceforge?

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

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


Re: [PATCHES] GIN improvements

2008-06-08 Thread Alvaro Herrera
Teodor Sigaev wrote:
 How about having a constant sized fastupdate buffer, of say 100 rows  
 or a fixed number of pages, and when that becomes full, the next  
 inserter will have to pay the price of updating the index and flushing 

 I'm not sure that is acceptable because flushing pending list may take 
 several seconds in unpredictable moment.

Perhaps we could make the fixed-size buffer be of size X, and trigger
autovac on X/3 or some such, to give it enough slack so that it would be
very unlikely to be processed by user processes.

 the buffer. To keep that overhead out of the main codepath, we could  
 make autovacuum to flush the buffers periodically.

 Do you mean that GIN sends a smoke signal to the autovacuum launcher 
 process to ask about vacuum?

Something like that, yes.

Currently, autovac only uses pgstats as trigger for actions.  Maybe we
could use something else (say, a flag in shared memory?), or just stash
the info that the index needs to be processed in pgstats and have
autovac examine it.

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

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


Re: [PATCHES] GIN improvements

2008-06-08 Thread Alvaro Herrera
Teodor Sigaev wrote:

 2) fast insert into GIN
 New version:
 http://www.sigaev.ru/misc/fast_insert_gin-0.6.gz

 Changes:
 - added option FASTUPDATE=(1|t|true|on|enable|0|f|false|disable) for
   CREATE/ALTER command for GIN indexes
 - Since there wasn't any comments on first email, pg_am.aminsertcleanup 
 optional
   method was introduced.

Hmm, this alters the semantics of amvacuumcleanup a bit.  Currently in
btvacuumcleanup we invoke btvacuumscan only if btbulkdelete was not
called.  This is noticed by observing whether the stats pointer is
NULL.  However, the patch changes this a bit because
index_vacuum_cleanup is called with the results of index_insert_cleanup,
instead of a plain NULL.

Right now this is not a problem because there is no insert_cleanup
function for btree, but I wonder if we should clean it up.

FWIW there's a typo in catalogs.sgml (finction - function)


What's the use of the FASTUPDATE parameter?  Is there a case when a user
is interested in turning it off?

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

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


Re: [PATCHES] [HACKERS] TRUNCATE TABLE with IDENTITY

2008-06-07 Thread Alvaro Herrera
Tom Lane wrote:

 2. I had first dismissed Neil's idea of transactional sequence updates
 as impossible, but on second look it could be done.  Suppose RESTART
 IDENTITY does this for each sequence;
 
   * obtain AccessExclusiveLock;
   * assign a new relfilenode;
   * insert a sequence row with all parameters copied except
 last_value copies start_value;
   * hold AccessExclusiveLock till commit.

Hmm, this kills the idea of moving sequence data to a single
non-transactional catalog :-(

 So what I think we should do is leave the patch there, revise the
 warning per Neil's complaint, and add a TODO item to reimplement RESTART
 IDENTITY transactionally.

I think the TODO item did not make it, but the docs do seem updated.

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

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


Re: [PATCHES] partial header cleanup

2008-06-01 Thread Alvaro Herrera
Zdenek Kotala wrote:
 This replace xlog.h with xlogdefs.h in bufpage.h. All other changes are  
 forgotten include somewhere. It reduce e.g. bloat to half in itup.h. But, 
 There are still unresolved problems. htup should include bufpage.h, 
 because it needs PageHeader size, but there is still unnecessary bufmgr.h 
 include in bufpage which generates bloat.

I agree with this patch -- in fact I had done the same before PGCon and
then neglected it for some reason.  (I think I was distracted trying to
get the struct RelationData definition out of rel.h, but that did not
turn out too well).

I was thinking maybe we need a third buffer manager header file.  One
would have the current bufmgr.h, another would have the page stuff that
does not know about bufmgr.h (so most of current bufpage.h), and the
third one would be both plus the #define that needs both (which is
currently in bufpage.h).  I am not sure what kind of fallout that
causes.  Maybe that would help you too.  We need to come up with a good
name for that file however ... bufmgrpage.h seems ugly.

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

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


Re: [PATCHES] \d+ should display the storage options for columns

2008-05-23 Thread Alvaro Herrera
Gregory Stark wrote:
 Alvaro Herrera [EMAIL PROTECTED] writes:
 
  This seems to be against an older version of psql ... with the
  printTable API stuff, we reworked this -- in particular the mbvalidate()
  call that's only on WIN32 is gone (actually it's the lack of it that's
  gone.)
 
 Sorry. Here's a patch against a current sync of HEAD.

Neat.

 Incidentally how can this new API work? Calling _() on a function parameter
 would work but how would the translation tools know what strings need to be
 translated?

Those strings need to be in gettext_noop().  (If we're missing that
somewhere else, it's a bug) :-)

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

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


Re: [PATCHES] Extending grant insert on tables to sequences

2008-05-23 Thread Alvaro Herrera
Jaime Casanova escribió:
 On Thu, May 22, 2008 at 1:18 PM, Jaime Casanova [EMAIL PROTECTED] wrote:
  Hi,
 
  The idea of this patch is to avoid the need to make explicit grants on
  sequences owned by tables.
 
 
 I've noted that the patch i attached is an older version that doesn't
 compile because of a typo...
 Re-attaching right patch and fix documentation to indicate the new 
 behaviour...

Please add the patch to the commitfest page,

http://wiki.postgresql.org/wiki/CommitFest:July

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

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


Re: [PATCHES] \d+ should display the storage options for columns

2008-05-22 Thread Alvaro Herrera
Gregory Stark wrote:
 
 Gregory Stark [EMAIL PROTECTED] writes:
 
  Oleg pointed out to me here that while we have a command to *set* the toast
  storage characteristics there's no actual supported way to display the 
  current
  settings.
 
  It seems like this would be a reasonable thing to add to \d+
 
 Sorry, sent the wrong diff before. The previous diff didn't work due to an
 array overflow.

This seems to be against an older version of psql ... with the
printTable API stuff, we reworked this -- in particular the mbvalidate()
call that's only on WIN32 is gone (actually it's the lack of it that's
gone.)


-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

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


Re: [PATCHES] Patch for psql 8.0, 8.1 and 8.2 backwards compatibility

2008-05-20 Thread Alvaro Herrera
Bryce Nesbitt wrote:
 Ugh, I started the wrong version of psql again.

 This patch offers basic backwards compatibility, so a version 8.4 psql  
 can successfully do common operations on Postgres 8.0, 8.1, 8.2 and 8.3.  
 I expect it's incomplete support, but as of yet I can't find an actual 
 problem.  To me it is a step forward regardless, as it fixes \d which 
 is pretty crucial even when just popping to an old server to check on 
 something before an upgrade.

Guillaume Lelarge just submitted a patch for this.  Did you check it
out?  It's on the July commitfest page.


-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

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


Re: [PATCHES] libpq object hooks

2008-05-16 Thread Alvaro Herrera
Andrew Dunstan escribió:

 All of this is getting quite a long way from what was in the commitfest  
 queue. Do we still want to try to get this in this cycle, or should it  
 be marked returned to author for more work?

There are still patches open for which no discussion has even started,
so I think this is OK for this commitfest.

(Besides, it seems we're almost there on this patch.)

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

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


Re: [PATCHES] Patch to change psql default banner v6

2008-05-15 Thread Alvaro Herrera
Andrew Dunstan wrote:

 Welcome to UI development. There is always *far* more argument of minor  
 matters of appearance than over anything else, in my experience.

Which is a good thing (in this case at least), because otherwise we
would end up with a crappy UI just because a single person thinks it's
good enough.

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

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


Re: [PATCHES] Patch to change psql default banner v6

2008-05-15 Thread Alvaro Herrera

I'm OK with thisG but please move the printSSLInfo() call just before
echoing the help line.

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

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


Re: [PATCHES] Patch to change psql default banner v6

2008-05-15 Thread Alvaro Herrera
Andrew Dunstan wrote:

 Second, it's not nearly as easy as that:
 . new commands have been added
 . postgres features have been added
 . catalogs have changed

Well, this just means a different piece of SQL needs to be sent for a
command depending on the server version, right?  It's not like that's
tremendously different.  The nice thing about most \X commands is that
they embed everything they need in a bunch of SQL, and they don't need
much else in C code.  So it's not all that difficult.

And for commands that have been added later, an initial version could
just say this server version does not support this command.  It would
be already a huge improvement.

Probably the biggest change would be to support versions that did not
have schemas, but I think it would be OK to punt on that.  We already
stopped supporting 7.2 anyway.

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

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


Re: [PATCHES] Patch to change psql default banner v6

2008-05-14 Thread Alvaro Herrera
Bruce Momjian wrote:

 If you type 'help' it just repeats the startup banner suggestion:
 
   test= help
   
   You are using psql, the command-line interface to PostgreSQL.
   Type \? for help.

I think we wanted to have more information in 'help', not less.  Making
it just repeat the startup info is not very helpful.

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

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


Re: [PATCHES] Patch to change psql default banner v6

2008-05-14 Thread Alvaro Herrera
Bruce Momjian wrote:

 I know we decided not to do that, but I am trying to figure out what the
 goal if 'help' is?  To display the most frequently-used help commands? 
 Aren't they at the top of \?.

The purpose of 'help' is to provide useful help.  If it only says see \?
then it's just redirecting you somewhere else, which isn't useful.

I don't think the various help commands need to be completely
orthogonal.  If you agree with that, then making 'help' repeat part of
what \? says is acceptable.  (Of course, the idea is not just to repeat,
but also to provide useful advice to the unwary.)

Remember, the people who is going to type 'help' is not the 10-year-Pg-
experience types.  It's the newbies.

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

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


Re: [PATCHES] Patch to change psql default banner v6

2008-05-14 Thread Alvaro Herrera
Bruce Momjian wrote:

 My question is whether we agreed that suggesting help as the best way
 to get help was what we agreed upon?  If we did, I forgot.  I thought
 the 'help' ideas was just for people who forgot the help commands.

Please review the previous discussion:

http://archives.postgresql.org/message-id/1200851790.19135.68.camel%40greg-laptop

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

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


Re: [PATCHES] libpq Win32 Mutex performance patch

2008-05-13 Thread Alvaro Herrera
Andrew Chernow wrote:
 Tom Lane wrote:
 Silently not locking is surely
 not very safe.


 Here is the dump code version of the patch.  If anyone wants the return  
 value idea, let me know.

So is this a patch we want applied?


-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

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


Re: [PATCHES] libpq Win32 Mutex performance patch

2008-05-13 Thread Alvaro Herrera
Magnus Hagander wrote:
 Alvaro Herrera wrote:
  Andrew Chernow wrote:
   Tom Lane wrote:
   Silently not locking is surely
   not very safe.
  
  
   Here is the dump code version of the patch.  If anyone wants the
   return value idea, let me know.
  
  So is this a patch we want applied?
 
 Please see my other thread about libpq thread-locking which should be
 finished before this one, after which this patch will change. So no,
 this is not the version to be applied.

Hmm, AFAICT on that other thread you state

 I'm leaning towards going with the simpler one, which is the patch
 from Andrew that crashes on out of memory.

which would seem to mean this patch?

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

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


Re: [PATCHES] [GENERAL] Making sure \timing is on

2008-05-13 Thread Alvaro Herrera
Tom Lane escribió:

 Actually, \a and \H are fairly bogus anyway, because they are toggling
 a setting that has more than two values.  I wonder whether defining the
 argument as a boolean is really very sane.  Perhaps it would be better to
 take the argument if given as just a regular format setting --- ie,
 with an argument, either of these would just be a shorthand for
 \pset format.

Agreed -- they are bogus.  However, making \H aligned be an alias for
\pset format aligned does not look very sane either (is that html
aligned or what?)

Seeing how these have been deprecated for a very long while, perhaps the
thing to do is leave their behavior alone and throw a warning when they
are used; in a couple more releases, remove them.  However this isn't
very sane either, because we'd break scripts that are using \H for no
good reason.

Another thing we could do is keep them as toggles, but instead of
aligned/HTML and aligned/unaligned, make them remember the state that
was set at the time they were called, and toggle between HTML (or
aligned) and the last state.

... except that since aligned is the default mode, \a should not toggle
between last state and aligned, but rather between last state and
unaligned.  Which makes it a misnomer.

Or maybe the thing to do is leave them damn well alone and just fix
\timing.

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

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


Re: [PATCHES] libpq object hooks

2008-05-13 Thread Alvaro Herrera
Andrew Dunstan escribió:

 The thing that is a bit disturbing is that the whole style of this  
 scheme is very different from the fairly simple APIs that the rest of  
 libpq presents. It's going to make libpq look rather odd, I think. I  
 would have felt happier if the authors had been able to come up with a  
 simple scheme to add API calls to export whatever information they  
 needed, rather than using this callback scheme.

I'm not sure I understand this point.  Remember that this is here to
support the libpqtypes library.  There doesn't seem to be a way for an
API such as you describe to work.

 Second, the hook names are compared case insensitively and by linear  
 search. I don't see any justification for using case insensitive names  
 for hooks in a C program, so I think that part should go. And if we  
 expect to keep anything other than trivial numbers of hooks we should  
 look at some sort of binary or hashed search.

Keep in mind that the original patch supported a single hook being
registered.  Perhaps we could dream about having a couple of hooks
registered, but turning into hashed search would seem to be overkill, at
least for now.  (If hooking into libpq is truly successful we can always
improve it later -- it's not an exported detail of the API after all.)

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

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


Re: [PATCHES] Snapshot management, final

2008-05-12 Thread Alvaro Herrera
Tom Lane wrote:

Patch committed, with most of these fixed.  On this item:

 AtSubCommit_Snapshot can leave us with multiple RegdSnapshotElt's for
 the same snap and s_level, which seems a bit bogus.

I agree it is bogus.  I punted though, and left the code as-is, with a
comment stating the problem.  We can revisit this problem later if it's
ever an issue.

Many thanks for the extensive help.

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

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


Re: [PATCHES] printTable API (was: Show INHERIT in \du)

2008-05-12 Thread Alvaro Herrera
Brendan Jurd escribió:

 Here's the latest version of the printTable API.  This version is
 against the current HEAD and merges in the changes made by the
 recently committed psql wrap patch.
 
 This version also includes Alvaro's fix for the issue of pg_strdup not
 being available to programs in scripts/ (as quoted below).

Thanks.  I had to merge Tom's fixes to the wrap code too.

Another thing that I changed is that now not null and default %s is
translatable in a table description.  Also, I removed the _() call
around things like ?%c? \%s.%s\, which is what we use for tables of
which we don't recognize the relkind.

Thanks for the patch.

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

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


Re: [PATCHES] Snapshot management, final

2008-05-11 Thread Alvaro Herrera
Tom Lane wrote:

I'm revising the patch; this comment is flawed though:

 Shouldn't UnregisterSnapshot insist that s_level be equal to current
 xact nest level?

It can't check that; consider

begin;
savepoint foo;
declare cur cursor for select (1), (2), (3);
savepoint bar;
close cur;
commit;

Thanks again for the review.

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

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


Re: [PATCHES] Snapshot management, final

2008-05-11 Thread Alvaro Herrera
Tom Lane wrote:
 Alvaro Herrera [EMAIL PROTECTED] writes:
  Tom Lane wrote:
  Shouldn't UnregisterSnapshot insist that s_level be equal to current
  xact nest level?
 
  It can't check that; consider
 
  begin;
  savepoint foo;
  declare cur cursor for select (1), (2), (3);
  savepoint bar;
  close cur;
  commit;
 
 Hmm ... but that close can't unregister the snapshot immediately,
 because you'd lose if the 2nd savepoint gets rolled back, no?  Is the
 handling of this case even correct at the moment?

No, CLOSE is not rolled back:

alvherre=# begin;
BEGIN
alvherre=# savepoint foo;
SAVEPOINT
alvherre=# declare cur cursor for select (1), (2), (3);
DECLARE CURSOR
alvherre=# savepoint bar;
SAVEPOINT
alvherre=# close cur;
CLOSE CURSOR
alvherre=# rollback to bar;
ROLLBACK
alvherre=# fetch all from cur;
ERREUR:  le curseur « cur » n'existe pas

Maybe this is possible to fix, but again I think it's outside the scope
of this patch.

 ISTM correct handling of this example would require that the close
 not really discard the snap until commit.  Then, given proper ordering
 of the cleanup operations at commit, you might be able to still have the
 cross-check about s_level in UnregisterSnapshot.  (IOW, maybe having
 snapshot cleanup be late in the commit sequence wasn't such a good
 choice...)

Right -- I'll move them earlier.  I don't think it's trivial to fix the
un-rollback-ability of CLOSE however.

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

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


Re: [PATCHES] Snapshot management, final

2008-05-10 Thread Alvaro Herrera
[Reposting with compressed patch]

Okay, I think I've fixed most of the issues in the reviewed patch.
Updated patch attached.

The most interesting change is that I've done away with CopySnapshot as
a public routine in favor of a new PushUpdatedSnapshot which does the
copy-update-push sequence.  Also I added a refcount to RegdSnapshotElt 
as suggested, and changed the subxact logic to substract that exact
amount on abort.

There's something I'm not sure what to do about:

Tom Lane wrote:

 Also, I think that the whole snapshot-sharing mechanism is not working
 as intended except for the serializable case; otherwise sequences
 like
   x = RegisterSnapshot(GetTransactionSnapshot());
   y = RegisterSnapshot(GetTransactionSnapshot());
 will result in x and y being separate copies.  Or are you assuming
 that this just isn't worth optimizing?

It's not that I don't think it's worth optimizing, but I think it's a
bit away from the scope of this patch.  The problem here is how to
notice that two consecutive GetTransactionSnapshot calls should really
return different snapshots, considering that shared state may change in
between.  Perhaps there's an easy way to optimize that; I don't know.

What does work is to get (say) a registered snapshot and push it as
active snapshot.  That results in a successfully shared snapshot.  For
example PortalStart does that for cursors, etc.


(FWIW another thing which is probably worth rethinking is the handling
of snapshots around PortalStart.  Some callers pass the currently active
snapshot; Others pass InvalidSnapshot.  Another passes an arbitrary
snapshot.  When it's Invalid, PortalStart calls GetTransactionSnapshot,
otherwise it uses the passed snap for PushActiveSnapshot.  So this is
all a bit confusing and wasteful and could use some clean up.  This is
material for a new patch however.)

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support


snapshot-9.patch.gz
Description: Binary data

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


Re: [PATCHES] [NOVICE] encoding problems

2008-05-09 Thread Alvaro Herrera
Bruce Momjian escribió:
 Guillaume Smet wrote:
  On Thu, May 8, 2008 at 9:11 PM, Bruce Momjian [EMAIL PROTECTED] wrote:

  As I mentioned it before, is there any chance for this fix to be
  backported to 8.3 branch? IMHO it's a usability regression.
 
 No, we don't change behaviors in back branches unless we get lots of
 complaints, and we haven't in this case.

complaints++

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

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


Re: [PATCHES] [NOVICE] encoding problems

2008-05-07 Thread Alvaro Herrera
Bruce Momjian wrote:

 Even if we knew the column position at output time, when we are doing
 aligned column width computations, we don't know the width of the
 previous columns so we would have no way to know how far the tab would
 extend in the current column.

If you start counting every line from the start of the current column,
it will align correctly regardless of the previous columns.

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

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


Re: [PATCHES] [NOVICE] encoding problems

2008-05-07 Thread Alvaro Herrera
Bruce Momjian wrote:
 Alvaro Herrera wrote:

  If you start counting every line from the start of the current column,
  it will align correctly regardless of the previous columns.
 
 At this stage you don't know the width of previous columns because you
 don't know if a very wide value is coming in a later row, so there is no
 way to output the width of the cell with a tab you are looking at now.
 
 Unless I am misunderstanding you.

Surely psql computes the width of all cells before printing anything.

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

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


Re: [PATCHES] [NOVICE] encoding problems

2008-05-07 Thread Alvaro Herrera
Bruce Momjian wrote:
 Alvaro Herrera wrote:

  Surely psql computes the width of all cells before printing anything.
 
 It does, but if you have a value that has a tab, how do you know what
 tab stop you are on because you don't know the final width of the
 previous columns at that time, so there is no way to know the width of
 that cell.

My point is that you don't need to align the tabstops with the start of
the line, but with the start of the _column_.  So the width of the
previous column doesn't matter.

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

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


Re: [PATCHES] Snapshot management, final

2008-05-07 Thread Alvaro Herrera
Tom Lane wrote:

 I looked over this patch and I think it still needs work.

Thanks for the thorough review.  I'll be working on these problems soon.

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

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


Re: [PATCHES] printTable API (was: Show INHERIT in \du)

2008-05-07 Thread Alvaro Herrera
Brendan Jurd escribió:

 On Thu, Apr 17, 2008 at 7:27 AM, Alvaro Herrera  wrote:

   Thanks.  I looked the patch over and did some minor changes.  Modified
   version attached.
 
 Cool, I had a look through your changes and they all seemed fine to
 me.  In particular, moving the header comments to the ... header ...
 file seemed like a smart move. =)

FWIW I just noticed something else.  With this patch we add pg_strdup
calls into print.c.  pg_strdup lives in common.c.

This is fine as psql is concerned, but we have another problem which is
that in bin/scripts there are two programs that want to use
printQuery().  The problem is that there's no pg_strdup there :-(

The easy solution is to add pg_strdup to bin/scripts/common.c, but there
we don't have a global progname, so the error report in the out of
memory case cannot carry the name of the program crashing.

I don't like that, but I don't see any other solution.  Ideas welcome.

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

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


Re: [PATCHES] [HACKERS] bug in numeric_power() function

2008-05-07 Thread Alvaro Herrera
Bruce Momjian wrote:
 Tom Lane wrote:
  Bruce Momjian [EMAIL PROTECTED] writes:
   I have developed the attached patch which fixes 0 ^ 123.3.
  
  Did you actually read the wikipedia entry you cited?

But that's about 0^0, not about 0^123.3.  See this other subsection:

http://en.wikipedia.org/wiki/Exponentiation#Powers_of_zero

0^123.3 is 0, not 1.

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

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


Re: [PATCHES] [HACKERS] bug in numeric_power() function

2008-05-07 Thread Alvaro Herrera
Bruce Momjian wrote:

 Ah, got it, and I updated the patch to remove the commment about
 discrete.

The page also says that 0^x is undefined when x is negative, not sure
about that one but I don't see it in your patch.

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

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


Re: [PATCHES] configure option for XLOG_BLCKSZ

2008-05-05 Thread Alvaro Herrera
Andreas 'ads' Scherbaum wrote:
 On Sat, 03 May 2008 13:14:35 -0400 Tom Lane wrote:
 
  Simon Riggs [EMAIL PROTECTED] writes:
  
   Not seen any gains from varying the WAL file size since then... 
  
  I think the use-case for varying the WAL segment size is unrelated to
  performance of the master server, but would instead be concerned with
  adjusting the granularity of WAL log shipping.
 
 *nod* I heard this argument several times. Simon: there was a discussion
 about this topic in Prato last year. Since WAL logfiles are usually
 binary stuff, the files can't be compressed much so a smaller logfile
 size on a not-so-much-used system would save a noticeable amount of
 bandwith (and cpu cycles for compression).

Seems the stuff to zero out the unused segment tail would be more useful
here.

Kevin sent me the source file some time ago -- he didn't want to upload
them to pgfoundry because he was missing a Makefile.  I built one for
him, but last time I looked he hadn't uploaded anything.

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

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


Re: [PATCHES] pg_postmaster_reload_time() patch

2008-05-02 Thread Alvaro Herrera
George Gensure escribió:

 So if nobody's got any further objections, could this patch be applied?

It's in the queue:

http://wiki.postgresql.org/wiki/CommitFest:May

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

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


Re: [PATCHES] Exposing keywords to clients

2008-05-02 Thread Alvaro Herrera
Dave Page wrote:
 Hi,
 
 The attached patch implements a new function, pg_get_keywords(), which
 returns a set of records describing the keywords recognised by the
 server. This allows clients such as pgAdmin to get quoting rules
 correct, and helps with other tasks such as syntax highlighting where
 we need to support multiple server versions.

FWIW pg_dump has fmtId() which does something related.

I think it's a bit bogus to be using the list as compiled client-side,
precisely due to the theoretical chance that it could change from one
server version to the next, but it's probably not very likely that we
ever remove a keyword from the server grammar.  And highlighting a
keyword that's not really a keyword is unlikely to be a problem in
practice -- in fact it makes it obvious that the user is likely to be in
trouble later when they upgrade.


 postgres=# select * from pg_get_keywords();
word|   category
 ---+---
  all   | Reserved
  binary| Type or function name
  xmlserialize  | Column name
  zone  | Unreserved
 (372 rows)
 
 I wasn't sure about the best way to describe the categories -
 obviously they need to be non-translatable (for client software to
 interpret), but human readable is also nice. I'm happy to hear
 alternate suggestions.

Perhaps use a separate string for machine parse (say R, T, C, U), and
let the string be translatable.


-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

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


Re: [PATCHES] pg_postmaster_reload_time() patch

2008-04-30 Thread Alvaro Herrera
George Gensure escribió:
 I've done a quick write up for reload time reporting from the
 administration TODO.  I was a little paranoid with the locking, but
 didn't want problems to occur with signals on the postmaster and the
 read side.

I'd say too much -- postmaster runs with signals blocked all the time
(except during select()) so this is not necessary there.

Regarding the locking on backends, I admit I am not sure if this is
really a problem enough that you need a spinlock for it.  Anyway we tend
not to use spinlocks too much -- probably an LWLock would be more
apropos, if a lock is really needed.  (A bigger question is whether the
reload time should be local for each backend, or exposed globally
through MyProc.  I don't think it's interesting enough to warrant that,
but perhaps others think differently.)

Lastly, I didn't read the patch close enough to tell if it would work on
both the EXEC_BACKEND case and the regular one.

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

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


Re: [PATCHES] Documentation: ALTER ROLE - no password

2008-04-30 Thread Alvaro Herrera
Andreas 'ads' Scherbaum wrote:
 
 Hello,
 
 i've seen the question how to remove a password now several times in
 the last weeks. Attached is a small patch which add a new example for
 the ALTER ROLE documentation.

Applied, thanks.

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

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


Re: [PATCHES] [HACKERS] Proposed patch - psql wraps at window width

2008-04-30 Thread Alvaro Herrera
Gregory Stark wrote:
 Alvaro Herrera [EMAIL PROTECTED] writes:

  I think that could be fixed easily by having the syntax be something
  like
 
  \pset format aligned:80
  \pset format aligned:autowrap
 
 I suppose. It seems kind of inconvenient though, what advantage does it have?

Over what?

I think having a separate \pset format wrapped does not make much
sense.  The fact that it's wrapped is not a new format in itself, just a
property of the aligned format.

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

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


Re: [PATCHES] [HACKERS] Proposed patch - psql wraps at window width

2008-04-29 Thread Alvaro Herrera
Bruce Momjian wrote:
 Bryce Nesbitt wrote:
  Bruce Momjian wrote:
   I have updated the documentation for this patch.  I consider it ready to
   apply.  I think it is as close to a middle ground as we are going to
   get.  Further adjustment will have to happen when we have more reports
   from the field.
 
  I heard a pretty compelling argument to make wrapped part of aligned,
  and thus I think the patch  is ready to go only after adjusting the
  user-facing syntax:

I think this is what makes the most sense of what I've seen so far.
Wrapped is a special case of aligned anyway (there's no wrapped
unaligned or wrapped HTML for example, nor would we want there to
be.)

  Output format is aligned, no wrapping.
  
  \pset format aligned autowrap
  Output format is aligned, with automatic wrapping to the window width for 
  terminals.
  
  \pset format aligned 80
  Output format is aligned, with a target width of 80 characters.

 Uh, well, we can do that, though looking at the psql code \pset only
 wants two arguments.

I think that could be fixed easily by having the syntax be something
like

\pset format aligned:80
\pset format aligned:autowrap

etc.

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

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


Re: [PATCHES] generate_subscripts

2008-04-28 Thread Alvaro Herrera
Pavel Stehule escribió:

 This patch contains generate_subscripts functions, that generate
 series of array's subscripts of some dimension:

Committed, thanks.

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

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


Re: [PATCHES] Removing typename from A_Const (was: Empty arrays with ARRAY[])

2008-04-28 Thread Alvaro Herrera
Brendan Jurd escribió:

 Here's my attempt to remove the typename field from A_Const.  There
 were a few places (notably flatten_set_variable_args() in guc.c, and
 typenameTypeMod() in parse_type.c) where the code expected to see an
 A_Const with a typename, and I had to adjust for an A_Const within a
 TypeCast.  Nonetheless, there was an overall net reduction of 34 lines
 of code, so I think this was a win.

Do say ... why don't we do away with A_Const altogether and just replace
it with Value?  After this patch, I don't see what's the difference.

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

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


Re: [HACKERS] [PATCHES] Removing typename from A_Const (was: Empty arrays with ARRAY[])

2008-04-28 Thread Alvaro Herrera
Tom Lane escribió:
 Alvaro Herrera [EMAIL PROTECTED] writes:
  Brendan Jurd escribi�:
  Here's my attempt to remove the typename field from A_Const.  There
  were a few places (notably flatten_set_variable_args() in guc.c, and
  typenameTypeMod() in parse_type.c) where the code expected to see an
  A_Const with a typename, and I had to adjust for an A_Const within a
  TypeCast.  Nonetheless, there was an overall net reduction of 34 lines
  of code, so I think this was a win.
 
  Do say ... why don't we do away with A_Const altogether and just replace
  it with Value?  After this patch, I don't see what's the difference.
 
 They're logically different things, and after I get done putting a parse
 location field into A_Const, they'll still be physically different too.

Aha.  Are you working from Brendan's patch?  I was going to commit it.

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

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


Re: [HACKERS] [PATCHES] Removing typename from A_Const (was: Empty arrays with ARRAY[])

2008-04-28 Thread Alvaro Herrera
Tom Lane escribió:
 Alvaro Herrera [EMAIL PROTECTED] writes:
  Tom Lane escribió:
  They're logically different things, and after I get done putting a parse
  location field into A_Const, they'll still be physically different too.
 
  Aha.  Are you working from Brendan's patch?  I was going to commit it.
 
 Sure, go ahead.  I was going to add the parse location at the same time,
 but it can perfectly well be done as a separate patch.

I came up with the attached patch.  I added the location bits (although
I am unsure if I got the locations right in the parser), but they are
unused -- figuring out how to use them would take me longer than I can
to spend on this.

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Index: src/backend/nodes/copyfuncs.c
===
RCS file: /home/alvherre/Code/cvs/pgsql/src/backend/nodes/copyfuncs.c,v
retrieving revision 1.392
diff -c -p -r1.392 copyfuncs.c
*** src/backend/nodes/copyfuncs.c	14 Apr 2008 17:05:33 -	1.392
--- src/backend/nodes/copyfuncs.c	28 Apr 2008 20:20:44 -
*** _copyAConst(A_Const *from)
*** 1639,1645 
  			break;
  	}
  
! 	COPY_NODE_FIELD(typename);
  
  	return newnode;
  }
--- 1639,1645 
  			break;
  	}
  
! 	COPY_SCALAR_FIELD(location);
  
  	return newnode;
  }
Index: src/backend/nodes/equalfuncs.c
===
RCS file: /home/alvherre/Code/cvs/pgsql/src/backend/nodes/equalfuncs.c,v
retrieving revision 1.321
diff -c -p -r1.321 equalfuncs.c
*** src/backend/nodes/equalfuncs.c	14 Apr 2008 17:05:33 -	1.321
--- src/backend/nodes/equalfuncs.c	28 Apr 2008 16:17:02 -
*** _equalParamRef(ParamRef *a, ParamRef *b)
*** 1691,1701 
  static bool
  _equalAConst(A_Const *a, A_Const *b)
  {
! 	if (!equal(a-val, b-val))		/* hack for in-line Value field */
! 		return false;
! 	COMPARE_NODE_FIELD(typename);
! 
! 	return true;
  }
  
  static bool
--- 1691,1697 
  static bool
  _equalAConst(A_Const *a, A_Const *b)
  {
! 	return equal(a-val, b-val);		/* hack for in-line Value field */
  }
  
  static bool
Index: src/backend/nodes/outfuncs.c
===
RCS file: /home/alvherre/Code/cvs/pgsql/src/backend/nodes/outfuncs.c,v
retrieving revision 1.325
diff -c -p -r1.325 outfuncs.c
*** src/backend/nodes/outfuncs.c	13 Apr 2008 20:51:20 -	1.325
--- src/backend/nodes/outfuncs.c	28 Apr 2008 17:24:10 -
*** _outAConst(StringInfo str, A_Const *node
*** 1945,1951 
  
  	appendStringInfo(str,  :val );
  	_outValue(str, (node-val));
! 	WRITE_NODE_FIELD(typename);
  }
  
  static void
--- 1945,1951 
  
  	appendStringInfo(str,  :val );
  	_outValue(str, (node-val));
! 	WRITE_INT_FIELD(location);
  }
  
  static void
Index: src/backend/parser/gram.y
===
RCS file: /home/alvherre/Code/cvs/pgsql/src/backend/parser/gram.y,v
retrieving revision 2.612
diff -c -p -r2.612 gram.y
*** src/backend/parser/gram.y	14 Apr 2008 17:05:33 -	2.612
--- src/backend/parser/gram.y	28 Apr 2008 19:48:16 -
*** static bool QueryIsRule = FALSE;
*** 91,101 
  
  static Node *makeColumnRef(char *relname, List *indirection, int location);
  static Node *makeTypeCast(Node *arg, TypeName *typename);
! static Node *makeStringConst(char *str, TypeName *typename);
! static Node *makeIntConst(int val);
! static Node *makeFloatConst(char *str);
! static Node *makeAConst(Value *v);
! static A_Const *makeBoolAConst(bool state);
  static FuncCall *makeOverlaps(List *largs, List *rargs, int location);
  static void check_qualified_name(List *names);
  static List *check_func_name(List *names);
--- 91,102 
  
  static Node *makeColumnRef(char *relname, List *indirection, int location);
  static Node *makeTypeCast(Node *arg, TypeName *typename);
! static Node *makeStringConst(char *str, int location);
! static Node *makeStringConstCast(char *str, TypeName *typename, int location);
! static Node *makeIntConst(int val, int location);
! static Node *makeFloatConst(char *str, int location);
! static Node *makeAConst(Value *v, int location);
! static Node *makeBoolAConst(bool state, int location);
  static FuncCall *makeOverlaps(List *largs, List *rargs, int location);
  static void check_qualified_name(List *names);
  static List *check_func_name(List *names);
*** set_rest:	/* Generic SET syntaxes: */
*** 1112,1118 
  	n-kind = VAR_SET_VALUE;
  	n-name = client_encoding;
  	if ($2 != NULL)
! 		n-args = list_make1(makeStringConst($2, NULL));
  	else
  		n-kind = VAR_SET_DEFAULT;
  	$$ = n;
--- 1113,1119 
  	n-kind = VAR_SET_VALUE;
  	n-name = client_encoding;
  	if ($2 != NULL)
! 		n-args = list_make1(makeStringConst($2, @2));
  	else

Re: [HACKERS] [PATCHES] Removing typename from A_Const (was: Empty arrays with ARRAY[])

2008-04-28 Thread Alvaro Herrera
Alvaro Herrera escribió:
 Tom Lane escribió:
  Alvaro Herrera [EMAIL PROTECTED] writes:
   Tom Lane escribió:
   They're logically different things, and after I get done putting a parse
   location field into A_Const, they'll still be physically different too.
  
   Aha.  Are you working from Brendan's patch?  I was going to commit it.
  
  Sure, go ahead.  I was going to add the parse location at the same time,
  but it can perfectly well be done as a separate patch.
 
 I came up with the attached patch.  I added the location bits (although
 I am unsure if I got the locations right in the parser), but they are
 unused -- figuring out how to use them would take me longer than I can
 to spend on this.

Hmm, I'm now wondering if the location should be added to Value as well,
so that it can be passed down to Const?

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

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


Re: [HACKERS] [PATCHES] Removing typename from A_Const (was: Empty arrays with ARRAY[])

2008-04-28 Thread Alvaro Herrera
Tom Lane escribió:
 Alvaro Herrera [EMAIL PROTECTED] writes:
  I came up with the attached patch.
 
 I wasn't envisioning anything anywhere near this invasive.  We only
 need locations on constants in a few contexts, I think.

Aha.  OK, I'll commit the original patch and let you deal with the rest
of it :-)

 BTW, you broke _equalAConst() ... it was a bad idea anyway to recast
 it on the assumption that there would never again be more than one
 field.

Oops, sorry, I had intended to revert that part and forgot.

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

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


Re: [PATCHES] lc_time and localized dates

2008-04-24 Thread Alvaro Herrera
Euler Taveira de Oliveira wrote:

 Here is an updated patch. It follows the Oracle behaviour and uses a  
 cache mechanism to avoid calling setlocale() all the time. I unified the  
 localized_* and str_* functions.  I didn't test it on Windows. I would  
 appreciate some feedback.

Added to May commitfest.



-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

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


Re: [PATCHES] 64-bit CommandIds

2008-04-24 Thread Alvaro Herrera
Bruce Momjian wrote:

  I think the case for it got a whole lot weaker in 8.3, with lazy
  consumption of CIDs.
 
 Agreed.  Let's see if we get requests for it in = 8.3 releases.

In the original submission message you find this text:

: attached is our patch against HEAD which enables extending CommandIds
: to 64-bit. This is for enabling long transactions that really do that
: much non-read-only work in one transaction.

Question for Hans-Juergen and Zoltan: have you tested 8.3 and do you
still see the need for this?

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

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


Re: [PATCHES] Snapshot management, final

2008-04-23 Thread Alvaro Herrera
Simon Riggs wrote:
 On Tue, 2008-04-22 at 18:13 -0400, Alvaro Herrera wrote:
  Simon Riggs wrote:
  
   OK, so it can;t be copied to a longer lived memory context?
  
  If you need that ability, please explain.
 
 No, I wish to prevent that, not enable it.

I see.  Sure, we don't have that problem.  In fact, we didn't have it
before either, so I'm not sure I see your point :-)

 Perhaps put the TransactionId on each snapshot and then an Assert can
 check it before its used. 

There's no need for that -- all snapshots go away at transaction end.
An attempt to use one would cause a prompt crash (at least on an
assert-enabled build.)

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

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


Re: [PATCHES] Snapshot management, final

2008-04-23 Thread Alvaro Herrera
Tom Lane wrote:

 The only reason we have memory contexts at all is to avoid the need to
 track individual palloc'd objects.  Since we're instituting exactly such
 tracking for snapshots, there's no value in placing them in
 general-purpose memory contexts.

FWIW I noticed yesterday after going to bed that SnapshotContext serves
no useful purpose -- we can just remove it and store snaps in
TopTransactionContext.

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

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


Re: [PATCHES] Snapshot management, final

2008-04-23 Thread Alvaro Herrera
Simon Riggs wrote:

 Forgive me for being dense, but what is there to stop you using a
 CopySnapshot in TopMemoryContext? If you did, there would be no way to
 free it, nor would we notice it had been done, AFAICS. Not anything I'm
 thinking about doing, though.

Well, TopMemoryContext is no good because we want to free snapshots in a
fell swoop at transaction abort.  TopTransactionContext would be OK, as
I just said in the parallel subthread.

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

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


Re: [PATCHES] temporal version of generate_series()

2008-04-23 Thread Alvaro Herrera
H.Harada escribió:

 # This is my first time to send a patch. If I did something wrong, I
 appreciate your pointing me out.

Brace positioning is off w.r.t. our conventions -- please fix that and
resubmit.

I have added this patch to the May commitfest.

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

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


Re: [PATCHES] Improve shutdown during online backup, take 4

2008-04-23 Thread Alvaro Herrera
Magnus Hagander wrote:

 Applied with some minor changes to the error messages to make them
 closer to our guidelines. (With my track record, it's not entirely
 unlikely that someone will fix them further though :-P)

I think the messages should not have a newline in the middle.

Also, I am wondering if in PM_WAIT_BACKUP mode we should accept new
connections from superusers only.

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

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


Re: [PATCHES] Snapshot management, final

2008-04-23 Thread Alvaro Herrera
Alvaro Herrera wrote:

 FWIW I noticed yesterday after going to bed that SnapshotContext serves
 no useful purpose -- we can just remove it and store snaps in
 TopTransactionContext.

... which is what the attached patch does.

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support


snapshot-7.patch.bz2
Description: Binary data

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


Re: [PATCHES] Removing NONSEG mode

2008-04-22 Thread Alvaro Herrera
Zdenek Kotala wrote:
 I attach patch which remove nonsegment mode support. It was discussed during
 last commit fest. Nonsegment mode is possible uses only on couple of FS (ZFS,
 XFS) and it is not safe on any OS because each OS support more filesystems.

Added to May commitfest, thanks.

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

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


[PATCHES] Snapshot management, final

2008-04-22 Thread Alvaro Herrera
Here is the patch for snapshot management I currently have.

Below is a complete description of the changes in this patch.

The most important change is on the use of CopySnapshot and the games
with ActiveSnapshot.  On the new code, whenever a piece of code needs a
snapshot to persist, it is registered by using RegisterSnapshot.  As
soon as it is done with it, it must be unregistered with
UnregisterSnapshot.

For ActiveSnapshot, we deal with a stack structure.  Callers needing an
Active snap set call PushActiveSnapshot, and PopActiveSnapshot when they
no longer need it.

GetSnapshotData still creates initial versions of each snapshot in
static memory.  There's a new boolean on the SnapshotData struct (set to
false by GetSnapshotData), which indicates whether the snapshot has been
copied out of static memory.  When a snapshot is Registered or
PushedActive, this bit is checked: if false, then a copy of the snapshot
is made on the dedicated SnapshotContext, and the copied flag is
flipped.

SnapshotData also carries two new reference counts: one for the Register
list, another one for the ActiveSnapshot stack.  Whenever a snap is
Unregistered or Popped, both refcounts are checked.  If both are found
to be zero then memory can be released.

On Unregister and Pop, after deleting the snapshot, we also verify the
whole lists.  If they are empty, it means no more live snapshot remain
for this transaction.  In this case we can reset MyProc-xmin to
InvalidXid.  Thus, GetSnapshotData must now recalculate MyProc-xmin
each time it finds xmin to be Invalid, which can not only happen for the
serializable snapshot but at any time.

A serializable transaction Registers its snapshot as first thing, and
keeps it registered until transaction end.


Note: I had previously made noises about changing the semantics of
MyProc-xmin or introducing a new VacuumXmin.  I have refrained from
doing so in this patch.


Those are the high-level changes.  Some coding changes:

- SerializableSnapshot and LatestSnapshot as global symbols are no more.

- A couple of PG_TRY blocks have been removed.  Particularly, on
plancache.c this means the do_planning() routine is now pointless and
has been folded back into its sole caller.

- Three CopySnapshot call sites remain outside snapmgr.c: DoCopy() on
copy.c, ExplainOnePlan() on explain.c and _SPI_execute_plan() on spi.c.
They are there because they grab the current ActiveSnapshot, modify it,
and then use the resulting snapshot.  There is no corresponding
FreeSnapshot, because it's not needed.

- CommandCounterIncrement now calls into snapmgr.c to set the CID of the
static snapshots.

- CREATE INDEX CONCURRENTLY, VACUUM FULL, and CLUSTER must now
explicitely Pop the ActiveSnapshot set by PortalRunUtility before
calling CommitTransactionCommand.

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.


snapshot-6.patch.bz2
Description: Binary data

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


Re: [PATCHES] Snapshot management, final

2008-04-22 Thread Alvaro Herrera
Simon Riggs wrote:
 On Tue, 2008-04-22 at 15:49 -0400, Alvaro Herrera wrote:
 
  - Three CopySnapshot call sites remain outside snapmgr.c: DoCopy() on
  copy.c, ExplainOnePlan() on explain.c and _SPI_execute_plan() on spi.c.
  They are there because they grab the current ActiveSnapshot, modify it,
  and then use the resulting snapshot.  There is no corresponding
  FreeSnapshot, because it's not needed.
 
 Not needed? How can we be certain that the modified snapshot does not
 outlive its original source?

It's not CopySnapshot that's not needed, but FreeSnapshot.  The point
here is that the snapshot will be freed automatically as soon as it is
PopActiveSnapshot'd out of existance.  CopySnapshot creates a new,
separate copy of the passed snapshot, and each of them will be freed
(separately) as soon as their refcounts reach zero.

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

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


Re: [PATCHES] Snapshot management, final

2008-04-22 Thread Alvaro Herrera
Simon Riggs wrote:

 OK, so it can;t be copied to a longer lived memory context?

CopySnapshot always copies snapshots to SnapshotContext, which is a
context that lives until transaction end.  There's no mechanism for
copying a snapshot into another context, because I don't see the need.

If you need that ability, please explain.

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

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


Re: [PATCHES] Snapshot management, final

2008-04-22 Thread Alvaro Herrera
Tom Lane wrote:
 Alvaro Herrera [EMAIL PROTECTED] writes:
 
  CopySnapshot always copies snapshots to SnapshotContext, which is a
  context that lives until transaction end.  There's no mechanism for
  copying a snapshot into another context, because I don't see the need.
 
 The only reason we have memory contexts at all is to avoid the need to
 track individual palloc'd objects.  Since we're instituting exactly such
 tracking for snapshots, there's no value in placing them in
 general-purpose memory contexts.

The problem is that we reuse snapshots, and not all uses have the same
longevity.  If a context goes away from under a snapshot and there are
other references to it, the result is a dangling pointer somewhere.
That's why we have reference counts on snaps: we know we can free one
when its refcounts are zero.  At the same time, the snapshots all go
away at transaction end with TopTransactionContext.

The other possible approach to this problem is creating a separate copy
each time a snapshot is reused, but this just causes extra palloc'ing
for no gain at all.

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

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


Re: [PATCHES] Proposed patch - psql wraps at window width

2008-04-21 Thread Alvaro Herrera
Bruce Momjian wrote:

 ! /* print a divider, middle columns only 
 */
 ! if ((j + 1) % col_count)
   {
 ! if (opt_border == 0)
 ! fputc(' ', fout);
 ! /* first line for values? */
 ! else if (line_count == 0  
 col_line_count == 0)
 ! fputs( | , fout);
 ! /* next value is beyond height? 
 */
 ! else if (line_count = 
 heights[j + 1])
 ! fputs(   , fout);
 ! /* start of another newline 
 string? */
 ! else if (col_line_count == 0)
 ! fputs( : , fout);
 ! else
 ! {
 ! /* Does the next column 
 wrap to this line? */
 ! struct lineptr 
 *this_line = col_lineptrs[j+1][line_count];
 ! boolstring_done = 
 *(this_line-ptr + bytes_output[j+1]) == 0;
 ! 
 ! fputs(string_done ?
  :  ; , fout);
 ! }
   }

I think it's a bad idea to use the same  :  separator in the two last
cases.  They are different and they should be displayed differently.

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

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


Re: [PATCHES] Proposed patch - psql wraps at window width

2008-04-21 Thread Alvaro Herrera
Alvaro Herrera wrote:
 Bruce Momjian wrote:
 

  !   fputs(string_done ?
   :  ; , fout);
  !   }
  }
 
 I think it's a bad idea to use the same  :  separator in the two last
 cases.  They are different and they should be displayed differently.

Bruce noted by IM that I misread the ? : expression :-)  Sorry for the
noise.

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

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


Re: [PATCHES] Testing pg_terminate_backend()

2008-04-21 Thread Alvaro Herrera
Magnus Hagander wrote:

 It's been running fine now for a number of hours, with output that
 looks similar to the stuff you posted. I'll leave it running..

Perhaps it would be a good idea to leave it running on code with some
bugs on it, just to check if the problems show up.

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

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


Re: [PATCHES] float4/float8/int64 passed by value with tsearch fixup

2008-04-19 Thread Alvaro Herrera
Zoltan Boszormenyi wrote:

 So I think we really do need to offer that compile-time option.
 Failing to do so will be tantamount to desupporting v0 functions
 altogether, which I don't think we're prepared to do.
 

 I have asked the Cybertec guys for a patch.  Since it's basically a copy
 of the float8 change, it should be easy to do.

 Here's the patch (against current CVS) with the required change.
 Please review, it passed make check with both --enable and  
 --disable-float4-byval.

Does it pass make installcheck in contrib?  I'm worried about
btree_gist in particular.  Perhaps the change I introduced in the
previous revision needs to be #ifdef'd out?

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

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


Re: [PATCHES] Coding standards

2008-04-18 Thread Alvaro Herrera
Bryce Nesbitt wrote:
 Alvaro Herrera wrote:
 People [are] complaining here that we don't teach people here anyway, so
 hopefully my comments were still useful :-)
   
 Yes they are useful.  As a new patcher, where should I look for coding  
 standards?  How about a little FAQ at the
 top of the CVS source tree?

The developer's FAQ is supposed to contain this kind of thing, but I
think it's rather thin on actual details.  (Some time ago it was
proposed to create a style guide, but people here thought it was a
waste of time and it will not cover what's really important, so we're
stuck with repeating the advice over and over.)

 Though, darn it, I sure like //

 And my vi is set to:
  set sw=4
  set ts=4
  set expandtab
 Because my corporate projects require spaces not tabs.

I use this:

:if match(getcwd(), /home/alvherre/Code/CVS/pgsql) == 0 
:  set cinoptions=(0
:  set tabstop=4
:  set shiftwidth=4
:  let $CSCOPE_DB=substitute(getcwd(), ^\\(.*/pgsql/source/[^/]*\\)/.*, 
\\1, )
:  let tags=substitute(getcwd(), ^\\(.*/pgsql/source/[^/]*\\)/.*, \\1, ) 
. /tags
:  let path=substitute(getcwd(), ^\\(.*/pgsql/source/[^/]*\\)/.*, \\1, ) 
. /src/include
:endif

Of course, you need to adjust the paths.  The cscope, tags and path
things let me quickly navigate through the files, but they don't affect
the editor behavior.

I never set expandtab so it's not a problem for me, but I guess you can
do :set noexpandtab in that block to reset it for Postgres use.

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

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


Re: [PATCHES] float4/float8/int64 passed by value with tsearch fixup

2008-04-18 Thread Alvaro Herrera
Zoltan Boszormenyi wrote:

 - float4 conversion is risk free (patch #1)

I applied this #1 patch.  It needed some further adjustments; in
particular contrib/btree_gist regression check was crashing, and
utils/fmgr/README needed updating.

With contrib/seg also adjusted to use float4 instead of float32, and
thus the last usage of float32 gone, I am now wondering if it would be a
good idea to remove the float32 and float32data definitions in c.h.

Thanks to Zoltan for the patch.

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

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


Re: [PATCHES] float4/float8/int64 passed by value with tsearch fixup

2008-04-18 Thread Alvaro Herrera
Alvaro Herrera wrote:

 With contrib/seg also adjusted to use float4 instead of float32, and
 thus the last usage of float32 gone, I am now wondering if it would be a
 good idea to remove the float32 and float32data definitions in c.h.

Ok, the buildfarm is going yellow over this change.  On cardinal I see
this:

*** ./expected/seg.out  Fri Apr 18 20:45:38 2008
--- ./results/seg.out   Fri Apr 18 20:59:40 2008
***
*** 1069,1218 
  SELECT seg_lower(s), seg_center(s), seg_upper(s)
  FROM test_seg WHERE s @ '11.2..11.3' OR s IS NULL ORDER BY s;
   seg_lower | seg_center | seg_upper 
! ---++---
!  -Infinity |  -Infinity |40
!  -Infinity |  -Infinity |82
!  -Infinity |  -Infinity |90
!  1 |  7 |13
!1.3 |   6.65 |12
!  2 |   6.75 |  11.5
!2.1 |   6.95 |  11.8
!2.3 |   Infinity |  Infinity
!2.3 |   Infinity |  Infinity
!2.4 |   6.85 |  11.3
!2.5 |  7 |  11.5
!2.5 |   7.15 |  11.8
!2.6 |   Infinity |  Infinity
!2.7 |   7.35 |12
!  3 |   Infinity |  Infinity
!  3 |   30.5 |58
!3.1 |7.3 |  11.5
!3.5 |7.5 |  11.5
!3.5 |   7.85 |  12.2
!  4 |  8 |12
!  4 |   Infinity |  Infinity
!  4 |  8 |12
!  4 |   7.85 |  11.7
!  4 |   8.25 |  12.5
!  4 |8.5 |13
!  4 | 32 |60
!  4 |   Infinity |  Infinity
!4.2 |   7.85 |  11.5
!4.2 |   7.95 |  11.7
!4.5 |   8.25 |12
!4.5 |  8 |  11.5
!4.5 |   8.25 |12
!4.5 |   8.25 |12
!4.5 |8.5 |  12.5
!4.5 |  59.75 |   115
!4.7 |   8.25 |  11.8
!4.8 |   8.15 |  11.5
!4.8 |8.2 |  11.6
!4.8 |   8.65 |  12.5
!4.8 |   Infinity |  Infinity
!4.9 |   8.45 |12
!4.9 |   Infinity |  Infinity
!  5 |   8.25 |  11.5
!  5 |8.5 |12
!  5 |   17.5 |30
!  5 |8.2 |  11.4
!  5 |   8.25 |  11.5
!  5 |8.3 |  11.6
!  5 |   8.35 |  11.7
!  5 |8.5 |12
!  5 |8.5 |12
!  5 |8.5 |12
!5.2 |   8.35 |  11.5
!5.2 |8.6 |12
!   5.25 |  8.625 |12
!5.3 |8.4 |  11.5
!5.3 |   9.15 |13
!5.3 |  47.65 |90
!5.3 |   Infinity |  Infinity
!5.4 |   Infinity |  Infinity
!5.5 |8.5 |  11.5
!5.5 |8.6 |  11.7
!5.5 |   8.75 |12
!5.5 |   8.75 |12
!5.5 |  9 |  12.5
!5.5 |9.5 |  13.5
!5.5 |   Infinity |  Infinity
!5.5 |   Infinity |  Infinity
!5.7 |   Infinity |  Infinity
!5.9 |   Infinity |  Infinity
!  6 |   8.75 |  11.5
!  6 |  9 |12
!  6 |   8.75 |  11.5
!  6 |9.5 |13
!  6 |   8.75 |  11.5
!6.1 |   9.05 |12
!6.1 |   Infinity |  Infinity
!6.2 |   8.85 |  11.5
!6.3 |   Infinity |  Infinity
!6.5 |  9 |  11.5
!6.5 |   9.25 |12
!6.5 |   9.25 |12
!6.5 |   Infinity |  Infinity
!6.6 |   Infinity |  Infinity
!6.7 |9.1 |  11.5
!6.7 |   Infinity |  Infinity
!   6.75 |   Infinity |  Infinity
!6.8 |   Infinity |  Infinity
!6.9 |   9.55 |  12.2
!6.9 |  48.45 |90
!6.9 |   Infinity |  Infinity
!  7 |   9.25 |  11.5
!  7 |   9.25 |  11.5
!  7 |   9.25 |  11.5
!  7 |   Infinity |  Infinity
!   7.15 |   Infinity |  Infinity
!7.2 |  10.35 |  13.5
!7.3 |  48.65 |90
!7.3 |   Infinity |  Infinity
!7.3 |   Infinity |  Infinity
!7.4 |   9.75 |  12.1
!7.4 |   Infinity |  Infinity
!7.5 |9.5 |  11.5
!7.5 |   9.75 |12
!7.5 |   Infinity |  Infinity
!7.7 |9.6 |  11.5
!7.7 |   Infinity |  Infinity
!   7.75 |   Infinity |  Infinity
!  8 |   9.85 |  11.7
!  8 | 10 |12
!  8 |   10.5 |13
!8.2 |   Infinity |  Infinity
!8.3 |   Infinity |  Infinity
!8.5 | 10 |  11.5
!8.5 |   10.5

Re: [PATCHES] float4/float8/int64 passed by value with tsearch fixup

2008-04-18 Thread Alvaro Herrera
Tom Lane wrote:
 Alvaro Herrera [EMAIL PROTECTED] writes:
  I assume this is just some dumb portability mistake on my part ... or
  perhaps the fact that the functions are still using v0 fmgr convention?
 
 Since they're v0, they'd have to explicitly know about the pass-by-ref
 status of float4.

Well, the previous code was doing some pallocs, and the new code is not:
http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/contrib/seg/seg.c.diff?r1=1.20;r2=1.21


 Did this patch include a compile-time choice of whether things could
 remain pass-by-ref?  I rather imagine that some people out there will
 prefer to stay that way instead of fix their old v0 code.

Hmm, nope.  Do we really need that?

I understand the backwards-compatibility argument, yet I wonder if it's
worth the extra effort and code complexity.

 In the meantime, converting contrib/seg to v1 might be the best
 solution.

Will do.

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

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


Re: [PATCHES] float4/float8/int64 passed by value with tsearch fixup

2008-04-18 Thread Alvaro Herrera
Tom Lane wrote:

 Specifically, I think what you missed is that on some platforms C
 functions pass or return float values differently from similar-sized
 integer or pointer values (typically, the float values get passed in
 floating-point registers).

Argh ... I would have certainly missed that.

 It'd be less ugly to convert to v1 calling convention.

Okay -- I'll change contrib/seg to v1 to greenify back the buildfarm.

 So I think we really do need to offer that compile-time option.
 Failing to do so will be tantamount to desupporting v0 functions
 altogether, which I don't think we're prepared to do.

I have asked the Cybertec guys for a patch.  Since it's basically a copy
of the float8 change, it should be easy to do.

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

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


Re: [PATCHES] Testing pg_terminate_backend()

2008-04-18 Thread Alvaro Herrera
Magnus Hagander wrote:
 Tom Lane wrote:

 SLEEP=`expr $RANDOM \* $REGRESSION_DURATION / 32767`
  
  Uh, where's the randomness coming from?
 
 ... but I should probably wait until that one is answered or fixed, I
 guess :-)

bash.

   RANDOM Each time this parameter is referenced, a random integer between
  0 and 32767 is generated.  The sequence of random numbers may be
  initialized by assigning a value to RANDOM.  If RANDOM is unset,
  it loses its special properties,  even  if  it  is  subsequently
  reset.


-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

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


Re: [PATCHES] float4/float8/int64 passed by value with tsearchfixup

2008-04-18 Thread Alvaro Herrera
!  8.98361e-038 | 8.98361e-038 | 8.98361e-038
!   8.9834e-038 |  8.9834e-038 |  8.9834e-038
!  9.03228e-038 | 9.03228e-038 | 9.03228e-038
!  9.03225e-038 | 9.03225e-038 | 9.03225e-038
!  8.98334e-038 | 8.98334e-038 | 8.98334e-038
!  8.98302e-038 | 8.98302e-038 | 8.98302e-038
!  8.98291e-038 | 8.98291e-038 | 8.98291e-038
!  8.98287e-038 | 8.98287e-038 | 8.98287e-038
!  8.98273e-038 | 8.98273e-038 | 8.98273e-038
!  9.04397e-038 | 9.04397e-038 | 9.04397e-038
!  8.98251e-038 | 8.98251e-038 | 8.98251e-038
!  8.98248e-038 | 8.98248e-038 | 8.98248e-038
!  8.98246e-038 | 8.98246e-038 | 8.98246e-038
!  8.98242e-038 | 8.98242e-038 | 8.98242e-038
!  9.03781e-038 | 9.03781e-038 | 9.03781e-038
!  8.98224e-038 | 8.98224e-038 | 8.98224e-038
!  8.98222e-038 | 8.98222e-038 | 8.98222e-038
!  8.98215e-038 | 8.98215e-038 | 8.98215e-038
|  | 
  (144 rows)

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

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


Re: [PATCHES] float4/float8/int64 passed by value with tsearchfixup

2008-04-18 Thread Alvaro Herrera
Tom Lane wrote:

 I was wondering about that too, once it became obvious that (almost?)
 everything was failing not just some platforms.  However, this
 afternoon's CVS HEAD *does* pass the seg regression test for me on HPPA,
 and I presume it passed on whatever Alvaro is using (btw, what was
 that?).  So there's definitely a platform dependency involved and not
 just you-missed-a-pointer-someplace.

Huh, this was with the code with v0 conventions, right?  I committed the
change to v1 conventions a while ago.

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

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


Re: [PATCHES] Proposed patch - psql wraps at window width

2008-04-17 Thread Alvaro Herrera
Bruce Momjian wrote:

 In testing I found the regression tests were failing because of a divide
 by zero error (fixed), and a missing case where the column delimiter has
 to be :.  In fact I now see all your line continuation cases using :
 rather than !.  It actually looks better --- ! was too close to |
 to be easily recognized.  (Did you update your patch to use :.  I
 didn't see ! in your patch.)

I think we should use a different separator when there is an actual
newline in the data.  Currently we have a : there, so using a : here is
probably not the best idea.

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

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


Re: [PATCHES] Proposed patch - psql wraps at window width

2008-04-17 Thread Alvaro Herrera
Bryce Nesbitt wrote:
 I've attached a patch, against current 8.4 cvs, which optionally sets a  
 maximum width for psql output:

Some random comments:

* Don't use C++ style comments (//).  Some compilers don't like these.

* Beware of brace position: we use braces on their own, indented at the
  start of a new line, so

!   while(--count) {
! lines++;
!   lines-ptr   = NULL;
!   lines-width = 0;
! }

becomes


!   while(--count)
!   {
! lines++;
!   lines-ptr   = NULL;
!   lines-width = 0;
! }

(with correct indentation anyway)


* Always use tabs, not spaces, to indent.  Tabs are 4 spaces wide.

* Don't use double stars in comments.

* We're not in the habit of giving credit in code comments.  It gets
messy fast.

* Don't lose warning comments like this one (unless you've removed the
assumption of course)

/*
 * Assumption: This code used only on strings
 * without multibyte characters, otherwise
 * this_line-width  strlen(this_ptr) and we get
 * an overflow
 */

In fact I wonder if you've introduced this assumption in the other case
on that code (i.e. when alignment is not 'r').  I'm not seeing any
checks for multibytes in there, but perhaps I'm missing it.


* } else is forbidden too.  Use two separate lines.

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

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


Re: [PATCHES] Proposed patch - psql wraps at window width

2008-04-17 Thread Alvaro Herrera
Bruce Momjian wrote:
 
 Alvaro is correct.  I made most or all of these adjustments in the
 updated version I posted yesterday.

Doh.  I didn't realize you had posted a new version :-(

People is complaining here that we don't teach people here anyway, so
hopefully my comments were still useful :-)

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

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


Re: [PATCHES] datum passed to macro which expects a pointer

2008-04-17 Thread Alvaro Herrera
Gavin Sherry wrote:
 Hi all,
 
 Attached are more fixes.

Applied, thanks.

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

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


  1   2   3   4   5   6   7   8   >