[PATCHES] [Fwd: Re: [GENERAL] Crosstab Problems]
Oops, just noticed I sent this to the General list instead of Patches --
sorry about that.
Joe
Original Message
Subject: Re: [GENERAL] Crosstab Problems
Date: Wed, 24 Oct 2007 19:26:16 -0700
From: Joe Conway <[EMAIL PROTECTED]>
To: Tom Lane <[EMAIL PROTECTED]>
CC: Jorge Godoy <[EMAIL PROTECTED]>, [EMAIL PROTECTED],
Stefan Schwarzer <[EMAIL PROTECTED]>
References: <[EMAIL PROTECTED]>
<[EMAIL PROTECTED]> <[EMAIL PROTECTED]>
<[EMAIL PROTECTED]> <[EMAIL PROTECTED]>
Tom Lane wrote:
Jorge Godoy <[EMAIL PROTECTED]> writes:
Em Thursday 18 October 2007 16:37:59 Joe Conway escreveu:
The row is pretty useless without a rowid in this context -- it seems
like the best thing to do would be to skip those rows entirely. Of
course you could argue I suppose that it ought to throw an ERROR and
bail out entirely. Maybe a good compromise would be to skip the row but
throw a NOTICE?
If I were using it and having this problem I'd rather have an ERROR.
I can think of four reasonably credible alternatives:
1. Treat NULL rowid as a category in its own right. This would conform
with the behavior of GROUP BY and DISTINCT, for instance.
> 4. Silently ignore rows with NULL rowid.
After looking closer I realized that #4 was my original intention, and
there was even code attempting to implement it, but just not very well ;-(.
In any case, the attached changes the behavior to #1 for both flavors of
crosstab (the original crosstab(text, int) and the usually more useful
crosstab(text, text)).
It is appropriate for 8.3 but not back-patching as it changes behavior
in a non-backward compatible way and is probably too invasive anyway.
I'll do something much simpler just to prevent crashing for 8.2 and earlier.
If there are no objections I'll apply Thursday.
Joe
Index: tablefunc.c
===
RCS file: /opt/src/cvs/pgsql/contrib/tablefunc/tablefunc.c,v
retrieving revision 1.47
diff -c -r1.47 tablefunc.c
*** tablefunc.c 3 Mar 2007 19:32:54 - 1.47
--- tablefunc.c 25 Oct 2007 02:11:06 -
***
*** 355,360
--- 355,361
crosstab_fctx *fctx;
int i;
int num_categories;
+ bool firstpass = false;
MemoryContext oldcontext;
/* stuff done only on the first call of the function */
***
*** 469,474
--- 470,476
funcctx->max_calls = proc;
MemoryContextSwitchTo(oldcontext);
+ firstpass = true;
}
/* stuff done on every call of the function */
***
*** 500,506
HeapTuple tuple;
Datum result;
char **values;
! bool allnulls = true;
while (true)
{
--- 502,508
HeapTuple tuple;
Datum result;
char **values;
! bool skip_tuple = false;
while (true)
{
***
*** 530,555
rowid = SPI_getvalue(spi_tuple, spi_tupdesc, 1);
/*
! * If this is the first pass through the values for this rowid
! * set it, otherwise make sure it hasn't changed on us. Also
! * check to see if the rowid is the same as that of the last
! * tuple sent -- if so, skip this tuple entirely
*/
if (i == 0)
- values[0] = pstrdup(rowid);
-
- if ((rowid != NULL) && (strcmp(rowid, values[0]) == 0))
{
! if ((lastrowid != NULL) && (strcmp(rowid, lastrowid) == 0))
break;
! else if (allnulls == true)
! allnulls = false;
/*
! * Get the next category item value, which is alway
* attribute number three.
*
! * Be careful to sssign the value to the array index based
* on which category we are presently processing.
*/
values[1 + i] = SPI_getvalue(spi_tuple, spi_tupdesc, 3);
--- 532,574
rowid = SPI_getvalue(spi_tuple, spi_tupdesc, 1);
/*
! * If this is the first pass through the values for this
! * rowid, set the first column to rowid
*/
if (i == 0)
{
! if (rowid)
! values[0] = pstrdup(rowid);
! else
! values[0] = NULL;
!
! /*
! * Check to see if the rowid is the same as that of the last
! * tuple sent -- if so, skip this tuple entirely
! */
! if (!firstpass &&
! (((lastrowid == NULL) && (rowid == NULL)) ||
! ((lastrowid != NULL) &&
! (rowid != NULL) &&
! (strcmp(rowid, lastrowid) == 0
! {
! skip_tuple = true;
break;
! }
! }
+ /*
+ * If rowid hasn't changed on us, continue building the
+ * ouput tuple.
+ */
+ if ((rowid && values[0] && (strcmp(rowid, values[0]) == 0)) ||
+ ((rowid == NULL) && (values[0] == NULL)))
+ {
/*
! * Get the next category item value, which is always
* attribute number three.
*
! * Be careful to assign the value to the array index based
* on which category we are presently processing.
*/
values[1 + i] = SPI_getvalue(spi_tup
Re: [PATCHES] vacuum as flags in PGPROC
Alvaro Herrera <[EMAIL PROTECTED]> writes: > Also, I forgot to mention it on the first email, but this patch adds > errcontext() lines when an autovacuum/analyze is being aborted. It > works fine, but I'm not seeing code anywhere else that does something > like this. This is a little bit scary because you might be invoking system catalog reads after the transaction has already failed. What would it take to save the names involved before starting the TRY block? I'm not worried about the errcontext() call per se, only about the syscache fetches. regards, tom lane ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] [HACKERS] Including Snapshot Info with Indexes
This has been saved for consideration for the 8.4 release: http://momjian.postgresql.org/cgi-bin/pgpatches_hold --- Gokulakannan Somasundaram wrote: > Hi, > I would like to present the first patch. It currently has the following > restrictions > a) It does not support any functional indexes. > b) It supports queries like select count(1) from table where (restrictions > from indexed columns), but it does not support select count(1) from table. > > The Syntax to create this type of index is > > create thick index idx on dd(n1,n2) > > here idx- index name and dd- table name and n1 and n2 are column names. > > I have created a extra column in pg_index called indhassnapshot. > > I have also enabled the display of Logical Reads. In order to see that, set > log_statement_stats on. > > The thick index is clearly on the front, if you issue queries like > > select n2 from dd where n1>1000 and n2<1500; > > As already said, if the update is not incurring any extra cost, except if > the indexed columns are updated. Deletes are costly, making it ideal for > partitioned tables. > > In order to update the thick indexes, i have accessed the ps_ExprContext in > PlanState to get the oldtuple. But if we have a outer plan and inner plan, > then i have set the ps_ExprContext of innerplan to the outerplan. I don't > know whether there will be instances where the ps_ExprContext of outerplan > node will have some use in update queries. > > Right now, it passes the regression test suite. I had slight trouble with > pg_indent, so i think it has not got applied properly. But i have tried to > remove all the whitespace differences. Please be kind to me in case i have > missed any whitespace differences. :) > > Please review the patch and provide your comments. > > Thanks, > Gokul. > CertoSQL Project, > Allied Solution Groups. > (www.alliedgroups.com) > > On 10/23/07, Hannu Krosing <[EMAIL PROTECTED]> wrote: > > > > ?hel kenal p?eval, L, 2007-10-20 kell 10:19, kirjutas Luke Lonergan: > > > Hi Hannu, > > > > > > On 10/14/07 12:58 AM, "Hannu Krosing" <[EMAIL PROTECTED]> wrote: > > > > > > > What has happened in reality, is that the speed difference between > > CPU, > > > > RAM and disk speeds has _increased_ tremendously > > > > > > Yes. > > > > > > > which makes it even > > > > more important to _decrease_ the size of stored data if you want good > > > > performance > > > > > > Or bring the cpu processing closer to the data it's using (or both). > > > > > > By default, the trend you mention first will continue in an unending way > > - > > > the consequence is that the "distance" between a processor and it's > > target > > > data will continue to increase ad-infinitum. > > > > the emergence of solid-state (flash) disks may help a little here, but > > in general it is true. > > > > > By contrast, you can only decrease the data volume so much - so in the > > end > > > you'll be left with the same problem - the data needs to be closer to > > the > > > processing. This is the essence of parallel / shared nothing > > architecture. > > > > > > Note that we've done this at Greenplum. We're also implementing a > > DSM-like > > > capability and are investigating a couple of different hybrid row / > > column > > > store approaches. > > > > Have you tried moving the whole visibility part of tuples out to a > > separate heap ? > > > > Especially in OLAP/ETL scenarios the distribution of tuples loaded in > > one transaction should be very good for visibility-info compression. > > > > I'd suspect that you could crush hundreds of pages worth of visibility > > into single RLE encoding unit (xmin=N, xmax=no_yet, start_ctid = X, > > end_ctid=Y), and it will stay in L1 cache most of the time you process > > the corresponding relation. and the relation itself will be smaller, and > > index-only (actually index-only + lookup inside L1 cache) access can > > happen, and so on . > > > > OTOH, if you load it in millions of small transactions, you can run > > VACUUM FREEZE _on_ the visibility heap only, which will make all > > visibility infoe look similar and thus RLE-compressable and again make > > it fit in L1 cache, if you dont have lots of failed loads interleaved > > with successful ones. > > > > > Bitmap index with index-only access does provide nearly all of the > > > advantages of a column store from a speed standpoint BTW. Even though > > > Vertica is touting speed advantages - our parallel engine plus bitmap > > index > > > will crush them in benchmarks when they show up with real code. > > > > > > Meanwhile they're moving on to new ideas - I kid you not "Horizontica" > > is > > > Dr. Stonebraker's new idea :-) > > > > Sounds like a result of a marketroid brainstorming session :P > > > > > So - bottom line - some ideas from column store make sense, but it's not > > a > > > cure-all. > > > > > > > There is also a MonetDB/X100 project, which tries to
Re: [PATCHES] vacuum as flags in PGPROC
Tom Lane wrote: > Heikki Linnakangas <[EMAIL PROTECTED]> writes: >> Alvaro Herrera wrote: >>> I did it that way (i.e. added locking) and then realized that it >>> shouldn't really be a problem, because the only one who can be setting >>> vacuum flags is the process itself. Other processes can only read the >>> flags. > >> It would still be a problem if there was any other fields that were >> updated by other processes, adjacent to the vacuum flags. I don't think >> that's the case, however. > > Well, that may not be the case today, but it still seems like an > assumption that will come back to bite us someday. And can you imagine > trying to debug a misbehavior like that? It's really not worth the risk, > given how seldom these flags will be changed. Oh, I totally agree. I wasn't trying to argue to the contrary. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] vacuum as flags in PGPROC
Heikki Linnakangas <[EMAIL PROTECTED]> writes: > Alvaro Herrera wrote: >> I did it that way (i.e. added locking) and then realized that it >> shouldn't really be a problem, because the only one who can be setting >> vacuum flags is the process itself. Other processes can only read the >> flags. > It would still be a problem if there was any other fields that were > updated by other processes, adjacent to the vacuum flags. I don't think > that's the case, however. Well, that may not be the case today, but it still seems like an assumption that will come back to bite us someday. And can you imagine trying to debug a misbehavior like that? It's really not worth the risk, given how seldom these flags will be changed. regards, tom lane ---(end of broadcast)--- TIP 2: Don't 'kill -9' the postmaster
Re: [PATCHES] vacuum as flags in PGPROC
Heikki Linnakangas wrote: > Alvaro Herrera wrote: > > I did it that way (i.e. added locking) and then realized that it > > shouldn't really be a problem, because the only one who can be setting > > vacuum flags is the process itself. Other processes can only read the > > flags. > > It would still be a problem if there was any other fields that were > updated by other processes, adjacent to the vacuum flags. I don't think > that's the case, however. Yeah, that's not the case currently. Tom is right in that it's fragile if we ever change the definition so that there is such a flag. Maybe this is solved by adding a comment however. -- Alvaro Herrera http://www.amazon.com/gp/registry/5ZYLFMCVHXC Jude: I wish humans laid eggs Ringlord: Why would you want humans to lay eggs? Jude: So I can eat them ---(end of broadcast)--- TIP 5: don't forget to increase your free space map settings
Re: [PATCHES] vacuum as flags in PGPROC
Alvaro Herrera wrote: > I did it that way (i.e. added locking) and then realized that it > shouldn't really be a problem, because the only one who can be setting > vacuum flags is the process itself. Other processes can only read the > flags. It would still be a problem if there was any other fields that were updated by other processes, adjacent to the vacuum flags. I don't think that's the case, however. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com ---(end of broadcast)--- TIP 2: Don't 'kill -9' the postmaster
Re: [PATCHES] vacuum as flags in PGPROC
Tom Lane wrote:
> Alvaro Herrera <[EMAIL PROTECTED]> writes:
> > I'm wondering if it's safe to do something like
> > MyProc->vacuumFlags |= PROC_FOR_XID_WRAPAROUND
> > without holding the ProcArrayLock.
>
> This seems a bit itchy.
>
> One thing I'd be worried about is processors that implement that by
> fetching the whole word containing the field, setting the bit, and
> storing back the whole word. (I believe some RISC processors are likely
> to do it like that.) This would mean that it'd work OK only as long as
> no other process was concurrently changing any field that happened to be
> in the same word, which is the kind of requirement that seems horribly
> fragile.
I did it that way (i.e. added locking) and then realized that it
shouldn't really be a problem, because the only one who can be setting
vacuum flags is the process itself. Other processes can only read the
flags.
Maybe the locking is not a problem anyway, but there are two additional
flag sets in analyze and two more in the autovacuum code when it detects
that it's vacuuming a table for Xid wraparound. (The idea is to use
these flags in the deadlock patch Simon posted, so that signals are
automatically sent or not according to the current activity of
autovacuum. This way the signal handler can be kept stupid.)
Also, I forgot to mention it on the first email, but this patch adds
errcontext() lines when an autovacuum/analyze is being aborted. It
works fine, but I'm not seeing code anywhere else that does something
like this.
--
Alvaro Herrerahttp://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Index: src/backend/access/transam/twophase.c
===
RCS file: /home/alvherre/Code/cvs/pgsql/src/backend/access/transam/twophase.c,v
retrieving revision 1.36
diff -c -p -r1.36 twophase.c
*** src/backend/access/transam/twophase.c 21 Sep 2007 16:32:19 - 1.36
--- src/backend/access/transam/twophase.c 24 Oct 2007 14:59:47 -
*** MarkAsPreparing(TransactionId xid, const
*** 283,290
gxact->proc.databaseId = databaseid;
gxact->proc.roleId = owner;
gxact->proc.inCommit = false;
! gxact->proc.inVacuum = false;
! gxact->proc.isAutovacuum = false;
gxact->proc.lwWaiting = false;
gxact->proc.lwExclusive = false;
gxact->proc.lwWaitLink = NULL;
--- 283,289
gxact->proc.databaseId = databaseid;
gxact->proc.roleId = owner;
gxact->proc.inCommit = false;
! gxact->proc.vacuumFlags = 0;
gxact->proc.lwWaiting = false;
gxact->proc.lwExclusive = false;
gxact->proc.lwWaitLink = NULL;
Index: src/backend/commands/analyze.c
===
RCS file: /home/alvherre/Code/cvs/pgsql/src/backend/commands/analyze.c,v
retrieving revision 1.109
diff -c -p -r1.109 analyze.c
*** src/backend/commands/analyze.c 24 Sep 2007 03:12:23 - 1.109
--- src/backend/commands/analyze.c 24 Oct 2007 14:59:47 -
***
*** 31,36
--- 31,37
#include "parser/parse_relation.h"
#include "pgstat.h"
#include "postmaster/autovacuum.h"
+ #include "storage/proc.h"
#include "utils/acl.h"
#include "utils/datum.h"
#include "utils/lsyscache.h"
*** analyze_rel(Oid relid, VacuumStmt *vacst
*** 201,206
--- 202,212
return;
}
+ /* let others know what I'm doing */
+ LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);
+ MyProc->vacuumFlags |= PROC_IN_ANALYZE;
+ LWLockRelease(ProcArrayLock);
+
/* measure elapsed time iff autovacuum logging requires it */
if (IsAutoVacuumWorkerProcess() && Log_autovacuum_min_duration >= 0)
{
*** analyze_rel(Oid relid, VacuumStmt *vacst
*** 484,489
--- 490,503
RelationGetRelationName(onerel),
pg_rusage_show(&ru0;
}
+
+ /*
+ * Reset my PGPROC flag. Note: we need this here, and not in vacuum_rel,
+ * because the vacuum flag is cleared by the end-of-xact code.
+ */
+ LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);
+ MyProc->vacuumFlags &= ~PROC_IN_ANALYZE;
+ LWLockRelease(ProcArrayLock);
}
/*
Index: src/backend/commands/vacuum.c
===
RCS file: /home/alvherre/Code/cvs/pgsql/src/backend/commands/vacuum.c,v
retrieving revision 1.359
diff -c -p -r1.359 vacuum.c
*** src/backend/commands/vacuum.c 20 Sep 2007 17:56:31 - 1.359
--- src/backend/commands/vacuum.c 24 Oct 2007 14:59:47 -
*** vacuum_set_xid_limits(int freeze_min_age
*** 660,668
* fixed-size never-null columns, but these are.
*
* Another reason for doing it this way is that when we are in a lazy
! * VACUUM and have inVacuum set, we mustn't do any updates --- somebody
! * vacuuming pg_class might think they could delete a tuple marked with
! * xmin = our xid.
*
* This routine is shared by full VACUUM, lazy VACUUM, and stand-alone
* ANA
Re: [PATCHES] vacuum as flags in PGPROC
Tom Lane wrote: > Alvaro Herrera <[EMAIL PROTECTED]> writes: > > In the spirit of incremental improvement, here is a patch that turns the > > couple of bools in PGPROC into a bitmask, and associated fallout. > > Maybe declare the field as uint8 instead of char? Otherwise, +1. I'm wondering if it's safe to do something like MyProc->vacuumFlags |= PROC_FOR_XID_WRAPAROUND without holding the ProcArrayLock. It seems in practice a type smaller than "int" (which uint8 always is) is always stored atomically so this shouldn't be a problem. -- Alvaro Herrera http://www.PlanetPostgreSQL.org/ "La rebeldÃa es la virtud original del hombre" (Arthur Schopenhauer) ---(end of broadcast)--- TIP 6: explain analyze is your friend
Re: [PATCHES] vacuum as flags in PGPROC
Alvaro Herrera <[EMAIL PROTECTED]> writes: > I'm wondering if it's safe to do something like > MyProc->vacuumFlags |= PROC_FOR_XID_WRAPAROUND > without holding the ProcArrayLock. This seems a bit itchy. One thing I'd be worried about is processors that implement that by fetching the whole word containing the field, setting the bit, and storing back the whole word. (I believe some RISC processors are likely to do it like that.) This would mean that it'd work OK only as long as no other process was concurrently changing any field that happened to be in the same word, which is the kind of requirement that seems horribly fragile. You could probably fix that by declaring the field as sig_atomic_t. Maybe better, just make it int. Another problem is that if you don't take a lock then there's no memory fence instructions executed, which would create issues around how soon other processors would see the effects of the change. While that might not be critical for the vacuum flags, it still seems a bit worrisome. Given that you're only changing these flags twice per vacuum, it doesn't seem worth taking any risks by not taking a lock ... regards, tom lane ---(end of broadcast)--- TIP 9: In versions below 8.0, the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
