Re: [PATCHES] BTree vacuum before page splitting

[Junji TERAMOTO]

Thanks!

I had misunderstood the content of the comment. I think that I can
understand the problem now.

I might be able to present the solution of the restarting an index scan.
I try to correct the patch.

Simon Riggs wrote:
> On Tue, 2006-01-31 at 15:18 +0900, Junji TERAMOTO wrote:
>> Tom Lane wrote:
>>> I think this is quite likely to break things :-(.  What sort of
>>> conditions have you tested it under?  (If this were safe, we'd
>>> not have invented the LP_DELETE flag to begin with, but just have
>>> deleted known-dead items immediately.)
>> I apologize for my insufficient description(and bad english).
>> I explain the operation of this patch in detail again.
>>
>> In _bt_insertonpg(), we insert the tupple by the following methods.
>>
>>  (1) Finds the right place(page) to insert the tuple.
>>  (2) If free space is insufficient, we operate as follows.
>> + (2.1) Confirm whether the lock to the found page is considerable
>> super-exclusive lock in BufferSuperLockHeldByMe()[new]. We
>> check bufHdr->refcount(=pin), and if pin == 1, we know this
>> lock is super-exclusive lock.
>> If our lock is not super-exclusive lock, goto (2.4).
>> + (2.2) If our lock is super-exclusive lock, and the found page is
>> leaf, we look for tupples marked as "LP_DELETE" from the found
>> page, and remove found tuples in _bt_vacuum_page()[new].
>> + (2.3) If free space is sufficient after removal, goto (4).
>>   (2.4) Step right to next non-dead page.
>>   (2.5) (2) is repeated until an enough free space is found or it
>> reaches a right edge at last.
>>  (3) When an enough free space is not found by processing (2), splits
>>  the target page (making sure that the split is equitable as far as
>>  post-insert free space goes).
>>  (4) Inserts the tuple.
>>
>> The numbers from 2.1) to 2.3) are new processing.
>>
>> The pointer's shifting by removing "LP_DELETE" tuples as shown in the
>> above-mentioned becomes a problem, when we resume a stopped indexscan.
>> Therefore, I am having it confirm the state of the lock by (2.1), and
>> process only at super-exclucive lock, same as btbulkdelete().
>>
>> However, because own indexscan might be lost because of this removal,
>> I also modify _bt_restscan() as follows.
>>
>>  (1) Check for index tuple we stopped on.
>>  (2) If index tuple is moved, first of all, we search in this page
>>  right.
>> +(3) If not found, we try to look for from the head of this page
>>  because index tuple may moved left due to removal.
>>  (4) If still not found, we look for index tuple right.
>>
>> The number (3) is new processing.
>>
>> We do not delete the empty page. Therefore, there is no necessity for
>> tracing a left page.
>>
>> I think that no problem occurs by this logic.
>> Please point it out if there is a wrong point. Thanks.
> 
> Please read comments in nbtree.c p.557-566 which describe the required
> locking protocol for btree pages.
> 
> Just because you hold the lock does *not* mean you are allowed to delete
> tuples marked as deleted. This patch doesn't work in all cases, which is
> what is required, since the failure cases don't raise an ERROR - they
> just miss data - which is unacceptable. So your patch works 99.9% of the
> time, but sometimes gives wrong answers to queries without you knowing.
> So, patch rejected, but good thinking all the same.
> 
> You will need to argue for a change in the locking protocol before it
> could be accepted. That would speed up VACUUM also, so if it were
> possible it would be gratefully accepted.
> 
> The only help I can give is this: Why does an index scan ever want to
> stop on a deleted tuple? Currently, the scan only remembers one tuple
> its seen. If the tuple was dead, but wasn't yet marked as dead the index
> scan will have read it and stopped there. Some while later, the index
> scan will return and begin scanning right for the tuple. If you delete
> it, the index scan *may* not be able to work out where to restart --
> ERROR. So, if you can work out a way of not restarting an index scan on
> a deleted tuple (and yet still marking them as deleted when it sees
> them) then you may be able to solve the problem.
> 
> I'm looking at the same problem domain also (table bloat), but focusing
> on a different approach that avoids this issue. 
> 
> Best Regards, Simon Riggs


-- 
Junji Teramoto / teramoto.junji (a) lab.ntt.co.jp

---(end of broadcast)---
TIP 3: Have you checked our extensive FAQ?

   http://www.postgresql.org/docs/faq

Re: [PATCHES] [HACKERS] FW: PGBuildfarm member snake Branch REL8_0_STABLE Status

[Bruce Momjian]

Dave Page wrote:
> Oops. 8.0, 8.1 and dev failed on Snake last night :-(

Fixed in all branches, patch attached.  I was unclear how the const was
going to behave for Win32 but it should be fine now.

Basically the const comes in via parameter, and we used to make a
non-const copy just for Win32; now we always do, but we still return it
as a const.

---


> 
> /D
> 
> 
> -Original Message-
> From: PG Build Farm [mailto:[EMAIL PROTECTED]
> Sent: Wed 2/1/2006 2:08 AM
> To: [EMAIL PROTECTED]; [EMAIL PROTECTED]
> Subject: PGBuildfarm member snake Branch REL8_0_STABLE Status changed from OK 
> to Make failure
>  
> 
> The PGBuildfarm member snake had the following event on branch REL8_0_STABLE:
> 
> Status changed from OK to Make failure
> 
> The snapshot timestamp for the build that triggered this notification is: 
> 2006-02-01 02:05:48
> 
> The specs of this machine are:
> OS:  Windows / Server 2003 SP1
> Arch: i686
> Comp: gcc / 3.4.2
> 
> For more information, see 
> http://www.pgbuildfarm.org/cgi-bin/show_history.pl?nm=snake&br=REL8_0_STABLE
> 
> 
> 
> 
> ---(end of broadcast)---
> TIP 1: if posting/reading through Usenet, please send an appropriate
>subscribe-nomail command to [EMAIL PROTECTED] so that your
>message can get through to the mailing list cleanly
> 

-- 
  Bruce Momjian|  http://candle.pha.pa.us
  pgman@candle.pha.pa.us   |  (610) 359-1001
  +  If your life is a hard drive, |  13 Roberts Road
  +  Christ can be your backup.|  Newtown Square, Pennsylvania 19073
Index: src/port/path.c
===
RCS file: /cvsroot/pgsql/src/port/path.c,v
retrieving revision 1.64
diff -c -c -r1.64 path.c
*** src/port/path.c 1 Feb 2006 00:31:59 -   1.64
--- src/port/path.c 1 Feb 2006 12:34:42 -
***
*** 389,395 
  get_progname(const char *argv0)
  {
const char  *nodir_name;
!   const char  *progname;
  
nodir_name = last_dir_separator(argv0);
if (nodir_name)
--- 389,395 
  get_progname(const char *argv0)
  {
const char  *nodir_name;
!   char*progname;
  
nodir_name = last_dir_separator(argv0);
if (nodir_name)

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

Re: [PATCHES] [BUGS] BUG #2221: Bad delimiters allowed in COPY ...

[Andrew Dunstan]

David Fetter wrote:


Anyhow, Bruce's patch still allows backslash as a delimiter, which can
cause *all* kinds of fun if not disallowed.


 



Yes, I'm puzzled by that. I can't really see any case for allowing it. 
And if we do, it should only be allowed in CSV mode, where its use will 
be merely perverse rather than broken.


cheers

andrew

---(end of broadcast)---
TIP 2: Don't 'kill -9' the postmaster

Re: [PATCHES] [HACKERS] FW: PGBuildfarm member snake Branch REL8_0_STABLE Status changed from OK to Make failure

[Dave Page]

Thanks Bruce.


-Original Message-
From: Bruce Momjian [mailto:[EMAIL PROTECTED]
Sent: Wed 2/1/2006 12:45 PM
To: Dave Page
Cc: PostgreSQL-patches
Subject: Re: [HACKERS] FW: PGBuildfarm member snake Branch REL8_0_STABLE Status 
changed from OK to Make failure
 
Dave Page wrote:
> Oops. 8.0, 8.1 and dev failed on Snake last night :-(

Fixed in all branches, patch attached.  I was unclear how the const was
going to behave for Win32 but it should be fine now.

Basically the const comes in via parameter, and we used to make a
non-const copy just for Win32; now we always do, but we still return it
as a const.

---


> 
> /D
> 
> 
> -Original Message-
> From: PG Build Farm [mailto:[EMAIL PROTECTED]
> Sent: Wed 2/1/2006 2:08 AM
> To: [EMAIL PROTECTED]; [EMAIL PROTECTED]
> Subject: PGBuildfarm member snake Branch REL8_0_STABLE Status changed from OK 
> to Make failure
>  
> 
> The PGBuildfarm member snake had the following event on branch REL8_0_STABLE:
> 
> Status changed from OK to Make failure
> 
> The snapshot timestamp for the build that triggered this notification is: 
> 2006-02-01 02:05:48
> 
> The specs of this machine are:
> OS:  Windows / Server 2003 SP1
> Arch: i686
> Comp: gcc / 3.4.2
> 
> For more information, see 
> http://www.pgbuildfarm.org/cgi-bin/show_history.pl?nm=snake&br=REL8_0_STABLE
> 
> 
> 
> 
> ---(end of broadcast)---
> TIP 1: if posting/reading through Usenet, please send an appropriate
>subscribe-nomail command to [EMAIL PROTECTED] so that your
>message can get through to the mailing list cleanly
> 

-- 
  Bruce Momjian|  http://candle.pha.pa.us
  pgman@candle.pha.pa.us   |  (610) 359-1001
  +  If your life is a hard drive, |  13 Roberts Road
  +  Christ can be your backup.|  Newtown Square, Pennsylvania 19073


---(end of broadcast)---
TIP 1: if posting/reading through Usenet, please send an appropriate
   subscribe-nomail command to [EMAIL PROTECTED] so that your
   message can get through to the mailing list cleanly

Re: [PATCHES] [BUGS] BUG #2221: Bad delimiters allowed in COPY ...

[Bruce Momjian]

David Fetter wrote:
> On Wed, Feb 01, 2006 at 01:16:08AM -0500, Tom Lane wrote:
> > Bruce Momjian  writes:
> > > Attached is a patch that errors for \r and \n in delimiter and
> > > null.  I kept the ERRCODE_FEATURE_NOT_SUPPORTED error code because
> > > that is what all the other error tests use in the copy code in
> > > that area.
> > 
> > I'd go with INVALID_PARAMETER_VALUE, I think.  ISTM that
> > FEATURE_NOT_SUPPORTED is appropriate for places where we might
> > someday support the case the error is rejecting.  For instance the
> > error just above your patch is for a multi-character delimiter
> > string.  That isn't completely senseless, it's just not implemented.
> > But we're not ever going to allow a delimiter setting that conflicts
> > with end-of-line, and I don't foresee allowing some other value for
> > end-of-line ;-) ... so this check isn't going to be removed someday.
> 
> I don't know why you're saying that the EOL character will never be
> changeable.  Other DBs (yes, I know that's not an argument for doing
> this, but please bear with me) let you set the "field separator" aka
> our DELIMITER and "record separator" aka our newline (or CRLF, in some
> cases. Oy!).
> 
> Anyhow, Bruce's patch still allows backslash as a delimiter, which can
> cause *all* kinds of fun if not disallowed.

OK, updated patch, which disallows backslash as a delimiter, and updated
error return code.

-- 
  Bruce Momjian|  http://candle.pha.pa.us
  pgman@candle.pha.pa.us   |  (610) 359-1001
  +  If your life is a hard drive, |  13 Roberts Road
  +  Christ can be your backup.|  Newtown Square, Pennsylvania 19073
Index: src/backend/commands/copy.c
===
RCS file: /cvsroot/pgsql/src/backend/commands/copy.c,v
retrieving revision 1.257
diff -c -c -r1.257 copy.c
*** src/backend/commands/copy.c 28 Dec 2005 03:25:32 -  1.257
--- src/backend/commands/copy.c 1 Feb 2006 14:05:53 -
***
*** 856,861 
--- 856,880 
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
 errmsg("COPY delimiter must be a single 
character")));
  
+   /* Disallow end-of-line characters */
+   if (strchr(cstate->delim, '\r') != NULL ||
+   strchr(cstate->delim, '\n') != NULL)
+   ereport(ERROR,
+   (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+errmsg("COPY delimiter cannot be newline or 
carriage return")));
+ 
+   if (strchr(cstate->null_print, '\r') != NULL ||
+   strchr(cstate->null_print, '\n') != NULL)
+   ereport(ERROR,
+   (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+errmsg("COPY null cannot use newline or 
carriage return")));
+ 
+   /* Disallow backslash in non-CSV mode */
+   if (!cstate->csv_mode && strchr(cstate->delim, '\\') != NULL)
+   ereport(ERROR,
+   (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+errmsg("COPY delimiter cannot be backslash")));
+ 
/* Check header */
if (!cstate->csv_mode && cstate->header_line)
ereport(ERROR,

---(end of broadcast)---
TIP 4: Have you searched our list archives?

   http://archives.postgresql.org

[PATCHES] test, please ignore

[Bruce Momjian]

test, please ignore

-- 
  Bruce Momjian|  http://candle.pha.pa.us
  pgman@candle.pha.pa.us   |  (610) 359-1001
  +  If your life is a hard drive, |  13 Roberts Road
  +  Christ can be your backup.|  Newtown Square, Pennsylvania 19073

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

Re: [PATCHES] [HACKERS] Bug: random() can return 1.0

[Bruce Momjian]

Tom Lane wrote:
> Bruce Momjian  writes:
> > Because random returns a double, I think it is very possible that we
> > could return 1 due to rounding,
> 
> Not unless your machine has a "double" type with less than 32 bits of
> precision, which seems pretty unlikely.  It'd be sufficient to do
> 
>   /* result 0.0 <= x < 1.0 */
>   result = ((double) random()) / ((double) MAX_RANDOM_VALUE + 1.0);

Here is a patch that makes this change, and cleans up other
MAX_RANDOM_VALUE uses.

-- 
  Bruce Momjian|  http://candle.pha.pa.us
  pgman@candle.pha.pa.us   |  (610) 359-1001
  +  If your life is a hard drive, |  13 Roberts Road
  +  Christ can be your backup.|  Newtown Square, Pennsylvania 19073
Index: src/backend/commands/analyze.c
===
RCS file: /cvsroot/pgsql/src/backend/commands/analyze.c,v
retrieving revision 1.90
diff -c -c -r1.90 analyze.c
*** src/backend/commands/analyze.c  22 Nov 2005 18:17:08 -  1.90
--- src/backend/commands/analyze.c  26 Jan 2006 23:40:49 -
***
*** 927,944 
return numrows;
  }
  
! /* Select a random value R uniformly distributed in 0 < R < 1 */
  static double
  random_fract(void)
  {
!   longz;
! 
!   /* random() can produce endpoint values, try again if so */
!   do
!   {
!   z = random();
!   } while (z <= 0 || z >= MAX_RANDOM_VALUE);
!   return (double) z / (double) MAX_RANDOM_VALUE;
  }
  
  /*
--- 927,937 
return numrows;
  }
  
! /* Select a random value R uniformly distributed in (0 - 1) */
  static double
  random_fract(void)
  {
!   return ((double) random() + 1) / ((double) MAX_RANDOM_VALUE + 2);
  }
  
  /*
Index: src/backend/storage/lmgr/s_lock.c
===
RCS file: /cvsroot/pgsql/src/backend/storage/lmgr/s_lock.c,v
retrieving revision 1.41
diff -c -c -r1.41 s_lock.c
*** src/backend/storage/lmgr/s_lock.c   22 Nov 2005 18:17:21 -  1.41
--- src/backend/storage/lmgr/s_lock.c   26 Jan 2006 23:40:54 -
***
*** 120,126 
  
/* increase delay by a random fraction between 1X and 
2X */
cur_delay += (int) (cur_delay *
! (((double) random()) / ((double) 
MAX_RANDOM_VALUE)) + 0.5);
/* wrap back to minimum delay when max is exceeded */
if (cur_delay > MAX_DELAY_MSEC)
cur_delay = MIN_DELAY_MSEC;
--- 120,126 
  
/* increase delay by a random fraction between 1X and 
2X */
cur_delay += (int) (cur_delay *
! ((double) random() / (double) 
MAX_RANDOM_VALUE) + 0.5);
/* wrap back to minimum delay when max is exceeded */
if (cur_delay > MAX_DELAY_MSEC)
cur_delay = MIN_DELAY_MSEC;
Index: src/backend/utils/adt/float.c
===
RCS file: /cvsroot/pgsql/src/backend/utils/adt/float.c,v
retrieving revision 1.119
diff -c -c -r1.119 float.c
*** src/backend/utils/adt/float.c   2 Dec 2005 02:49:11 -   1.119
--- src/backend/utils/adt/float.c   26 Jan 2006 23:40:57 -
***
*** 1833,1840 
  {
float8  result;
  
!   /* result 0.0-1.0 */
!   result = ((double) random()) / ((double) MAX_RANDOM_VALUE);
  
PG_RETURN_FLOAT8(result);
  }
--- 1833,1840 
  {
float8  result;
  
!   /* result [0.0 - 1.0) */
!   result = (double) random() / ((double) MAX_RANDOM_VALUE + 1);
  
PG_RETURN_FLOAT8(result);
  }
Index: src/include/optimizer/geqo_random.h
===
RCS file: /cvsroot/pgsql/src/include/optimizer/geqo_random.h,v
retrieving revision 1.16
diff -c -c -r1.16 geqo_random.h
*** src/include/optimizer/geqo_random.h 31 Dec 2004 22:03:36 -  1.16
--- src/include/optimizer/geqo_random.h 26 Jan 2006 23:40:58 -
***
*** 28,34 
  
  /* geqo_rand returns a random float value between 0 and 1 inclusive */
  
! #define geqo_rand() (((double) random()) / ((double) MAX_RANDOM_VALUE))
  
  /* geqo_randint returns integer value between lower and upper inclusive */
  
--- 28,34 
  
  /* geqo_rand returns a random float value between 0 and 1 inclusive */
  
! #define geqo_rand() ((double) random() / (double) MAX_RANDOM_VALUE)
  
  /* geqo_randint returns integer value between lower and upper inclusive */
  

---(end of broadcast)---
TIP 6: explain analyze is your friend

Re: [PATCHES] New pg_dump options: exclude tables/schemas, multiple

[Bruce Momjian]

Where are we on this patch?  Should we add code to do regex calls to the
backend using '~'.

---

Greg Sabino Mullane wrote:
> -BEGIN PGP SIGNED MESSAGE-
> Hash: SHA1
> NotDashEscaped: You need GnuPG to verify this message
>  
>  
> Attached is a patch to hopefully make pg_dump a lot more useful.
> I started out by making it simply able to avoid dumping a single
> table, but, inspired by David Fetter's patch last November, also
> added in support for multiple items and limited wildcard matching.
>  
> -n and -N control the schemas, and -t and -T control the tables.
>  
> Wildcards can be a star at the start, the end, or on both sides
> of a term. The patch acts inclusively with conflicts: the -t
> option trumps the -N option.
>  
> Some examples:
>  
> To dump all tables beginning with the string "slony", plus
> all tables with the word "log" inside of them:
>  
> pg_dump -t "slony*" -t "*log*"
>  
> To dump all schemas except "dev" and "qa", and all tables
> except those ending in "large":
>  
> pg_dump -N "dev" -N "qa" -T "*large"
>  
> --
> Greg Sabino Mullane [EMAIL PROTECTED]
> PGP Key: 0x14964AC8 200601152100
> http://biglumber.com/x/web?pk=2529DF6AB8F79407E94445B4BC9B906714964AC8
> -BEGIN PGP SIGNATURE-
>  
> iD8DBQFDyv9NvJuQZxSWSsgRAup9AKD110JJtJBYYPV5JxFROovfeddrSACg3IZ3
> BqczBImC8UCVmik3YFHvDeQ=
> =Y9zs
> -END PGP SIGNATURE-
> 

[ Attachment, skipping... ]

> 
> ---(end of broadcast)---
> TIP 4: Have you searched our list archives?
> 
>http://archives.postgresql.org

-- 
  Bruce Momjian|  http://candle.pha.pa.us
  pgman@candle.pha.pa.us   |  (610) 359-1001
  +  If your life is a hard drive, |  13 Roberts Road
  +  Christ can be your backup.|  Newtown Square, Pennsylvania 19073

---(end of broadcast)---
TIP 6: explain analyze is your friend

Re: [PATCHES] New pg_dump options: exclude tables/schemas, multiple

[Greg Sabino Mullane]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

  
> Where are we on this patch?  Should we add code to do regex calls
> to the backend using '~'.

Yes - I am planning to submit a new patch when I get a few spare
cycles. Hopefully no more than a few days from now.

- --
Greg Sabino Mullane [EMAIL PROTECTED]
PGP Key: 0x14964AC8 200602011424
http://biglumber.com/x/web?pk=2529DF6AB8F79407E94445B4BC9B906714964AC8
-BEGIN PGP SIGNATURE-

iD8DBQFD4QsBvJuQZxSWSsgRAs6XAKCCEobXLaOQfTf0PXpFTl0f90cNWQCgi6wO
g4F6pcP6mjjLYsdkjrKAvF8=
=jJFm
-END PGP SIGNATURE-



---(end of broadcast)---
TIP 2: Don't 'kill -9' the postmaster

Re: [PATCHES] pg_restore COPY error handling

[Stephen Frost]

* Bruce Momjian (pgman@candle.pha.pa.us) wrote:
> Stephen Frost wrote:
> > Great!  It'd be really nice to have this fix in 8.1.3... :)
> 
> No, it will not be in 8.1.3.  It is a new feature.

I'd really appriciate it if you could review my post to pgsql-bugs,
Message-ID: <[EMAIL PROTECTED]>.  If this patch is
considered a new feature then I'd like to either revisit that decision
or be given some direction on what a bug-fix patch for the grows-to-3G
bug would look like.

Thanks,

Stephen


signature.asc
Description: Digital signature

Re: [PATCHES] pg_restore COPY error handling

[Bruce Momjian]

Stephen Frost wrote:
-- Start of PGP signed section.
> * Bruce Momjian (pgman@candle.pha.pa.us) wrote:
> > Stephen Frost wrote:
> > > Great!  It'd be really nice to have this fix in 8.1.3... :)
> > 
> > No, it will not be in 8.1.3.  It is a new feature.
> 
> I'd really appriciate it if you could review my post to pgsql-bugs,
> Message-ID: <[EMAIL PROTECTED]>.  If this patch is
> considered a new feature then I'd like to either revisit that decision
> or be given some direction on what a bug-fix patch for the grows-to-3G
> bug would look like.

Oh, so when it skips the \., it reads the rest of the file and bombs
out?  I thought it just continued fine.  How is that failure happening?

-- 
  Bruce Momjian|  http://candle.pha.pa.us
  pgman@candle.pha.pa.us   |  (610) 359-1001
  +  If your life is a hard drive, |  13 Roberts Road
  +  Christ can be your backup.|  Newtown Square, Pennsylvania 19073

---(end of broadcast)---
TIP 3: Have you checked our extensive FAQ?

   http://www.postgresql.org/docs/faq

Re: [PATCHES] pg_restore COPY error handling

[Andrew Dunstan]

On Wed, 2006-02-01 at 17:20 -0500, Stephen Frost wrote:
> * Bruce Momjian (pgman@candle.pha.pa.us) wrote:
> > Stephen Frost wrote:
> > > Great!  It'd be really nice to have this fix in 8.1.3... :)
> > 
> > No, it will not be in 8.1.3.  It is a new feature.
> 
> I'd really appriciate it if you could review my post to pgsql-bugs,
> Message-ID: <[EMAIL PROTECTED]>.  If this patch is
> considered a new feature then I'd like to either revisit that decision
> or be given some direction on what a bug-fix patch for the grows-to-3G
> bug would look like.
> 


Stephen,

this is a hopeless way of giving a reference. Many users don't keep list
emails. If you want to refer to a previous post you should give a
reference to the web archives.

I assume you are referring to this post:
http://archives.postgresql.org/pgsql-bugs/2006-01/msg00188.php

cheers

andrew


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

Re: [PATCHES] pg_restore COPY error handling

[Bruce Momjian]

Andrew Dunstan wrote:
> On Wed, 2006-02-01 at 17:20 -0500, Stephen Frost wrote:
> > * Bruce Momjian (pgman@candle.pha.pa.us) wrote:
> > > Stephen Frost wrote:
> > > > Great!  It'd be really nice to have this fix in 8.1.3... :)
> > > 
> > > No, it will not be in 8.1.3.  It is a new feature.
> > 
> > I'd really appriciate it if you could review my post to pgsql-bugs,
> > Message-ID: <[EMAIL PROTECTED]>.  If this patch is
> > considered a new feature then I'd like to either revisit that decision
> > or be given some direction on what a bug-fix patch for the grows-to-3G
> > bug would look like.
> > 
> 
> 
> Stephen,
> 
> this is a hopeless way of giving a reference. Many users don't keep list
> emails. If you want to refer to a previous post you should give a
> reference to the web archives.
> 
> I assume you are referring to this post:
> http://archives.postgresql.org/pgsql-bugs/2006-01/msg00188.php

OK, that helps.  The solution is to "not do that", meaning install
postgis before the restore or something.  Anyway, adding the patch seems
like too large a risk for a minor release, especially since you are the
first person to complain about it that I remember.

-- 
  Bruce Momjian|  http://candle.pha.pa.us
  pgman@candle.pha.pa.us   |  (610) 359-1001
  +  If your life is a hard drive, |  13 Roberts Road
  +  Christ can be your backup.|  Newtown Square, Pennsylvania 19073

---(end of broadcast)---
TIP 6: explain analyze is your friend

Re: [PATCHES] pg_restore COPY error handling

[Tom Lane]

Bruce Momjian  writes:
> Andrew Dunstan wrote:
>> I assume you are referring to this post:
>> http://archives.postgresql.org/pgsql-bugs/2006-01/msg00188.php

> OK, that helps.  The solution is to "not do that", meaning install
> postgis before the restore or something.  Anyway, adding the patch seems
> like too large a risk for a minor release, especially since you are the
> first person to complain about it that I remember.

I don't think you should see this as something specific to PostGIS.
If I interpret Stephen's report properly, any sort of failure during
COPY IN would lead to undesirable behavior in pg_restore.

This is not super surprising because the original design approach for
pg_restore was "bomb out on any sort of difficulty whatsoever".  That
was justly complained about, and now it will try to keep going after
SQL-command errors ... but it sounds like the COPY-data processing
part didn't get fixed properly.

I take no position on whether Stephen's patch is any good --- I haven't
looked at it --- but if he's correct about the problem then I agree it's
a bug fix.  Before deciding whether it deserves to be back-patched, we
at least ought to look closely enough to characterize the situations in
which pg_restore will fail.

regards, tom lane

---(end of broadcast)---
TIP 5: don't forget to increase your free space map settings

Re: [PATCHES] pg_restore COPY error handling

[Stephen Frost]

* Bruce Momjian (pgman@candle.pha.pa.us) wrote:
> Stephen Frost wrote:
> -- Start of PGP signed section.
> > * Bruce Momjian (pgman@candle.pha.pa.us) wrote:
> > > Stephen Frost wrote:
> > > > Great!  It'd be really nice to have this fix in 8.1.3... :)
> > > 
> > > No, it will not be in 8.1.3.  It is a new feature.
> > 
> > I'd really appriciate it if you could review my post to pgsql-bugs,
> > Message-ID: <[EMAIL PROTECTED]>.  If this patch is
> > considered a new feature then I'd like to either revisit that decision
> > or be given some direction on what a bug-fix patch for the grows-to-3G
> > bug would look like.
> 
> Oh, so when it skips the \., it reads the rest of the file and bombs
> out?  I thought it just continued fine.  How is that failure happening?

It doesn't actually bomb out either.  On the system I was working with
what happened was that it hung after reading in the COPY data.  It
looked like it got stuck somehow while trying to parse the data as an
SQL command.

Thanks,

Stephen


signature.asc
Description: Digital signature

Re: [PATCHES] pg_restore COPY error handling

[Stephen Frost]

* Andrew Dunstan ([EMAIL PROTECTED]) wrote:
> this is a hopeless way of giving a reference. Many users don't keep list
> emails. If you want to refer to a previous post you should give a
> reference to the web archives.

Sorry, I'm actually pretty used to using Message IDs for references (we
do it alot on the Debian lists).  I'll try to be better in the future
though, honestly, I'm not really a big fan of the Postgres mailing list
archive setup. :/

> I assume you are referring to this post:
> http://archives.postgresql.org/pgsql-bugs/2006-01/msg00188.php

Yup, that's the one, thanks.

Stephen


signature.asc
Description: Digital signature

Re: [PATCHES] pg_restore COPY error handling

[Stephen Frost]

* Bruce Momjian (pgman@candle.pha.pa.us) wrote:
> Andrew Dunstan wrote:
> > this is a hopeless way of giving a reference. Many users don't keep list
> > emails. If you want to refer to a previous post you should give a
> > reference to the web archives.
> > 
> > I assume you are referring to this post:
> > http://archives.postgresql.org/pgsql-bugs/2006-01/msg00188.php
> 
> OK, that helps.  The solution is to "not do that", meaning install
> postgis before the restore or something.  Anyway, adding the patch seems
> like too large a risk for a minor release, especially since you are the
> first person to complain about it that I remember.

It's not PostGIS specific, it's any case where a COPY command fails for
any reason.  I'll be following up with the Debian maintainer regarding
being able to install things before doing the restore/pg_upgradecluster
(at the moment there's no hook from pg_upgradecluster between creating
the database and trying to load the data).  I understand that's not
really a PostgreSQL issue but I do think pg_restore should be able to
handle a COPY command failing sanely... :/

I didn't think the patch was terribly large or complicated...  The one
thing I don't like about it is that it doesn't seem to be very easy to
differentiate between a COPY command failing and some other SQL command
failing.  If there's a better way to detect this than to just check for
'COPY', I'd love to hear it. :)  I'd also be happy to make
any changes necessary to have the patch accepted, of course...

Thanks,

Stephen


signature.asc
Description: Digital signature

Re: [PATCHES] pg_restore COPY error handling

[Stephen Frost]

* Tom Lane ([EMAIL PROTECTED]) wrote:
> Bruce Momjian  writes:
> > Andrew Dunstan wrote:
> >> I assume you are referring to this post:
> >> http://archives.postgresql.org/pgsql-bugs/2006-01/msg00188.php
> 
> > OK, that helps.  The solution is to "not do that", meaning install
> > postgis before the restore or something.  Anyway, adding the patch seems
> > like too large a risk for a minor release, especially since you are the
> > first person to complain about it that I remember.
> 
> I don't think you should see this as something specific to PostGIS.
> If I interpret Stephen's report properly, any sort of failure during
> COPY IN would lead to undesirable behavior in pg_restore.

I'm reasonably confident this is exactly the case.

> This is not super surprising because the original design approach for
> pg_restore was "bomb out on any sort of difficulty whatsoever".  That
> was justly complained about, and now it will try to keep going after
> SQL-command errors ... but it sounds like the COPY-data processing
> part didn't get fixed properly.

Exactly so.  Possibly because it appears to be non-trivial to detect
when it was a *COPY* command which failed (to differentiate it from some
other command).  What I did was check for 'COPY'.  I'd be
happy to change that if anyone has a better suggestion though.  There
might be a way to pass that information down into the pg_archive_db but
I don't think it's available at that level currently and I didn't see
any way to get the answer from libpq about what *kind* of command
failed.

> I take no position on whether Stephen's patch is any good --- I haven't
> looked at it --- but if he's correct about the problem then I agree it's
> a bug fix.  Before deciding whether it deserves to be back-patched, we
> at least ought to look closely enough to characterize the situations in
> which pg_restore will fail.

I have some level of confidence that this is the only case along these
lines because it's the only case where pg_restore isn't dealing with SQL
commands but with a dataset which has to be handled in a special way.
pg_restore checks if the command sent was a successful COPY command and
then treats everything after it in a special way; it doesn't do that for
any other commands...

Thanks,

Stephen


signature.asc
Description: Digital signature

Re: [PATCHES] pg_restore COPY error handling

[Tom Lane]

Stephen Frost <[EMAIL PROTECTED]> writes:
> * Tom Lane ([EMAIL PROTECTED]) wrote:
>> This is not super surprising because the original design approach for
>> pg_restore was "bomb out on any sort of difficulty whatsoever".  That
>> was justly complained about, and now it will try to keep going after
>> SQL-command errors ... but it sounds like the COPY-data processing
>> part didn't get fixed properly.

> Exactly so.  Possibly because it appears to be non-trivial to detect
> when it was a *COPY* command which failed (to differentiate it from some
> other command).  What I did was check for 'COPY'.  I'd be
> happy to change that if anyone has a better suggestion though.

ISTM you should be depending on the archive structure: at some level,
at least, pg_restore knows darn well whether it is dealing with table
data or SQL commands.

Also, it might be possible to depend on whether libpq has entered the
"copy in" state.  I'm not sure this works very well, because if there's
an error in the COPY command itself (not in the following data) then we
probably don't ever get into "copy in" state.

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

Re: [PATCHES] pg_restore COPY error handling

[Andrew Dunstan]

Tom Lane said:
>
> Also, it might be possible to depend on whether libpq has entered the
> "copy in" state.  I'm not sure this works very well, because if there's
> an error in the COPY command itself (not in the following data) then we
> probably don't ever get into "copy in" state.
>


Could we not refrain from sending data if we detect that we are not in copy
in state?

cheers

andrew




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

Re: [PATCHES] pg_restore COPY error handling

[Stephen Frost]

* Andrew Dunstan ([EMAIL PROTECTED]) wrote:
> Tom Lane said:
> > Also, it might be possible to depend on whether libpq has entered the
> > "copy in" state.  I'm not sure this works very well, because if there's
> > an error in the COPY command itself (not in the following data) then we
> > probably don't ever get into "copy in" state.
> 
> Could we not refrain from sending data if we detect that we are not in copy
> in state?

You have to know at that level if it's data you're getting or an SQL
statement though.  SQL statements can fail and things move along
happily.  Essentially what my patch does is detect when a COPY command
fails and then circular-file the data that comes in after it from the
upper-level.  When I started to look at the way pg_restore worked I had
initially thought I could make the higher-level code realize that the
COPY command failed through a return-code and have it not pass the data
down but the return codes didn't appear to be very well defined to
handle that kind of communication...  (As in, it looked like trying to
make that happen would break other things), I can look at it again
though.

Thanks,

Stephen


signature.asc
Description: Digital signature

Re: [PATCHES] pg_restore COPY error handling

[Stephen Frost]

* Tom Lane ([EMAIL PROTECTED]) wrote:
> ISTM you should be depending on the archive structure: at some level,
> at least, pg_restore knows darn well whether it is dealing with table
> data or SQL commands.

Alright, to do this, ExecuteSqlCommandBuf would need to be modified to
return an error-code other than 1 for when a command fails.  Thankfully,
only pg_backup_archiver.c uses ExecuteSqlCommandBuf, in ahwrite.
ahwrite would then need to return a value when there's an error (at the
moment it's defined to return int but everything appears to ignore its
return value).  We'd then need ahprintf to return a negative value when
it fails (at the moment it returns 'cnt' which appears to be the size of
what was written, except that nothing looks at the return value of
ahprintf either).

Once we have ahprintf returning a negative value when it fails, we can
modify pg_backup_archiver.c around line 332 to check if the ahprintf
failed for a COPY statement and if so to skip the:
(*AH->PrintTocDataPtr) (AH, te, ropt); // on line 335.

I'd be happy to work this up, and I think it would work, but it seems
kind of ugly since then we'd have ahwrite and ahprintf returning error
codes which in 99% of the cases wouldn't be checked which doesn't seem
terribly clean. :/

Thanks,

Stephen


signature.asc
Description: Digital signature

Re: [PATCHES] pg_restore COPY error handling

[Stephen Frost]

* Stephen Frost ([EMAIL PROTECTED]) wrote:
> * Tom Lane ([EMAIL PROTECTED]) wrote:
> > ISTM you should be depending on the archive structure: at some level,
> > at least, pg_restore knows darn well whether it is dealing with table
> > data or SQL commands.
> 
[...]
> I'd be happy to work this up, and I think it would work, but it seems
> kind of ugly since then we'd have ahwrite and ahprintf returning error
> codes which in 99% of the cases wouldn't be checked which doesn't seem
> terribly clean. :/

Looking at this again, I'm not 100% sure it'd work quite that cleanly.
I'm not sure you can just skip that PrintTocDataPtr call, depending on
the how the input is coming in...  There might be a way to modify
_PrintData (in pg_backup_custom.c, and the others, which is what is
the function behind PrintTocDataPtr) to somehow check an AH variable
which essentially says "the data command failed, just ignore the input",
and we'd need to set the AH variable somewhere else, perhaps using the
return value setup I described before...

All of this is sounding rather invasive though.  I have to admit that
I'm not exactly a big fan of the pg_dump/pg_restore modular system. :/

Thanks,

Stephen


signature.asc
Description: Digital signature

Re: [PATCHES] pg_restore COPY error handling

[Bruce Momjian]

Stephen Frost wrote:
-- Start of PGP signed section.
> * Stephen Frost ([EMAIL PROTECTED]) wrote:
> > * Tom Lane ([EMAIL PROTECTED]) wrote:
> > > ISTM you should be depending on the archive structure: at some level,
> > > at least, pg_restore knows darn well whether it is dealing with table
> > > data or SQL commands.
> > 
> [...]
> > I'd be happy to work this up, and I think it would work, but it seems
> > kind of ugly since then we'd have ahwrite and ahprintf returning error
> > codes which in 99% of the cases wouldn't be checked which doesn't seem
> > terribly clean. :/
> 
> Looking at this again, I'm not 100% sure it'd work quite that cleanly.
> I'm not sure you can just skip that PrintTocDataPtr call, depending on
> the how the input is coming in...  There might be a way to modify
> _PrintData (in pg_backup_custom.c, and the others, which is what is
> the function behind PrintTocDataPtr) to somehow check an AH variable
> which essentially says "the data command failed, just ignore the input",
> and we'd need to set the AH variable somewhere else, perhaps using the
> return value setup I described before...

Whatever we do is going to be cleaner than looking for "*COPY".

> All of this is sounding rather invasive though.  I have to admit that
> I'm not exactly a big fan of the pg_dump/pg_restore modular system. :/

Hence TODO has:

o Remove unnecessary function pointer abstractions in pg_dump source
  code

-- 
  Bruce Momjian|  http://candle.pha.pa.us
  pgman@candle.pha.pa.us   |  (610) 359-1001
  +  If your life is a hard drive, |  13 Roberts Road
  +  Christ can be your backup.|  Newtown Square, Pennsylvania 19073

---(end of broadcast)---
TIP 3: Have you checked our extensive FAQ?

   http://www.postgresql.org/docs/faq

Re: [PATCHES] pg_restore COPY error handling

[Tom Lane]

Stephen Frost <[EMAIL PROTECTED]> writes:
> I'd be happy to work this up, and I think it would work, but it seems
> kind of ugly since then we'd have ahwrite and ahprintf returning error
> codes which in 99% of the cases wouldn't be checked which doesn't seem
> terribly clean. :/

I agree.  I wonder if it wouldn't be cleaner to pass the information in
the other direction, ie, send a boolean down to PrintTocData saying "you
are sending SQL commands" or "you are sending COPY data".  Then, instead
of depending only on the libpq state to decide what to do in
ExecuteSqlCommandBuf, we could cross-check: if we're sending SQL data
and the libpq state is wrong, just discard the line.

The immediate problem you saw is fairly clear at this point:
ExecuteSqlCommandBuf and its subroutines attempt to parse the data well
enough to determine command boundaries (if SQL commands) or line
boundaries (if COPY data).  If they are misinformed about what they are
processing then the parsing gets totally confused --- it's not hard to
imagine the code thinking the entire COPY dump is an incomplete SQL
command.  So driving this from an upper-level indication of what we are
currently sending, rather than libpq status, ought to be more robust.

BTW, we'd not need to mess with a ton of routine APIs to make this
happen --- just add a flag in ArchiveHandle.

regards, tom lane

---(end of broadcast)---
TIP 3: Have you checked our extensive FAQ?

   http://www.postgresql.org/docs/faq

Re: [PATCHES] [BUGS] BUG #2171: Differences compiling plpgsql in ecpg and psql

[Bruce Momjian]

I have researched your report, and you are right, there are two ecpg
bugs here.  First, dollar quoting uses single-quotes internally to do
the quoting, but it does not double any single-quotes in the
dollar-quoted string.

Second, when a dollar quoted string or single-quoted string spans
multiple lines, ecpg does not escape the newline that is part of the
string.  Some compilers will accept an unescaped newline in a string,
while others will not:

$ gcc -pedantic -c  -g -Wall tst1.c
tst1.c:5: warning: string constant runs past end of line

It isn't standard so I think we need to replace newline in a string with
"\n\".

Attached is a patch which fixes both of these issues.  This changes ecpg
behavior so I am thinking this patch would only appear in 8.2.

I am unclear if I fixed the \r case properly.

---

[EMAIL PROTECTED] wrote:
> 
> The following bug has been logged online:
> 
> Bug reference:  2171
> Logged by:  
> Email address:  [EMAIL PROTECTED]
> PostgreSQL version: 8.1.2
> Operating system:   Linux (Debian)
> Description:Differences compiling plpgsql in ecpg and psql
> Details: 
> 
> There appear to be parsing problems with ecpg.  The following example
> program shows code snippets that allow for the successful creation of a
> function (CREATE FUNCTION) only using two different syntaxes: one when
> entered through psql, and another when compiling with ecpg.
> 
> The expectation (and hints from the documentation) indicate that the exact
> same method of defining a function should succeed in both cases, but such is
> not the case.
> 
> Different quoting and line-wrap behavior is observed between psql and ecpg.
> 
> (Thanks for the attention, I hope this is useful!)
> 
> BEGIN CODE---
> /* This file is bug.pgc. */
> /* Compile as shown:
>ecpg   bug.pgc -o bug.c
>gcc -c -g -std=c99 -I/usr/local/pgsql/include -L/usr/local/pgsql/lib
> bug.c -o bug.o
>gcc -I/usr/local/pgsql/include -L/usr/local/pgsql/lib -lecpg bug.o -o bug
> */
> /* Run as: ./bug */
> #include 
> #include 
> #include 
> 
> int main(int argc, char* argv[]) {
> 
>   EXEC SQL CONNECT TO DEFAULT;
> 
>   EXEC SQL SET AUTOCOMMIT TO ON;
>   EXEC SQL WHENEVER SQLWARNING SQLPRINT;
>   EXEC SQL WHENEVER SQLERROR SQLPRINT;
> 
>   EXEC SQL CREATE TABLE My_Table ( Item1 int, Item2 text );
> 
>   /* Documentation appears to indicate that only single quotes (') are
>  needed, but this will not ecpg-compile without double-single ('')
>  quotes.  When entered through psql, only the single quotes (')
>  are needed. */
>   /* doc/html/sql-syntax.html#SQL-SYNTAX-DOLLAR-QUOTING: "It is
>  particularly useful when representing string constants inside
>  other constants, as is often needed in procedural function
>  definitions." */
>   /* doc/html/sql-createfunction.html: "Without dollar quoting, any
>  single quotes or backslashes in the function definition must be
>  escaped by doubling them." */
> 
>   /* Documentation appears to indicate that the body of the funtion
>  can be extended across multiple lines in the input file (this
>  file) but it will not compile (ecpg) without keeping the function
>  body on one line.  Multiple line input works through psql, but
>  not here.*/
> //bad ecpg,good psql: EXEC SQL CREATE FUNCTION My_Table_Check() RETURNS
> trigger
> //bad ecpg,good psql:   AS $My_Table_Check$
> //bad ecpg,good psql:   BEGIN RAISE NOTICE 'TG_NAME=%, TG WHEN=%', TG_NAME,
> TG_WHEN;
> //bad ecpg,good psql: RETURN NEW;
> //bad ecpg,good psql: END;
> //bad ecpg,good psql: $My_Table_Check$
> //bad ecpg,good psql:   LANGUAGE 'plpgsql';
>   EXEC SQL CREATE FUNCTION My_Table_Check() RETURNS trigger
> AS $My_Table_Check$ BEGIN RAISE NOTICE ''TG_NAME=%, TG WHEN=%'',
> TG_NAME, TG_WHEN; RETURN NEW; END; $My_Table_Check$
> LANGUAGE 'plpgsql';
> 
>   EXEC SQL CREATE TRIGGER My_Table_Check_Trigger
> BEFORE INSERT
> ON My_Table
> FOR EACH ROW
> EXECUTE PROCEDURE My_Table_Check();
> 
>   EXEC SQL INSERT INTO My_Table VALUES (1234, 'Some random text');
>   EXEC SQL INSERT INTO My_Table VALUES (5678, 'The Quick Brown');
> 
>   EXEC SQL DROP TRIGGER My_Table_Check_Trigger ON My_Table;
>   EXEC SQL DROP FUNCTION My_Table_Check();
>   EXEC SQL DROP TABLE My_Table;
> 
>   EXEC SQL DISCONNECT ALL;
> 
>   return 0;
> }
> 
> END CODE--
> 
> ---(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
> 

-- 
  Bruce Momjian|  http://candle.pha.pa.us
  pgman@candle.pha.pa.us   |  (610) 359-1001
  +  If your life is a hard drive, |  13 Roberts Road
  +  Christ can be your backup.|  Newtown Square, Pennsylvania 19073
Index: src/interfaces/

Re: [PATCHES] pg_restore COPY error handling

[Stephen Frost]

* Tom Lane ([EMAIL PROTECTED]) wrote:
> Stephen Frost <[EMAIL PROTECTED]> writes:
> > * Tom Lane ([EMAIL PROTECTED]) wrote:
> >> This is not super surprising because the original design approach for
> >> pg_restore was "bomb out on any sort of difficulty whatsoever".  That
> >> was justly complained about, and now it will try to keep going after
> >> SQL-command errors ... but it sounds like the COPY-data processing
> >> part didn't get fixed properly.
> 
> > Exactly so.  Possibly because it appears to be non-trivial to detect
> > when it was a *COPY* command which failed (to differentiate it from some
> > other command).  What I did was check for 'COPY'.  I'd be
> > happy to change that if anyone has a better suggestion though.
> 
> ISTM you should be depending on the archive structure: at some level,
> at least, pg_restore knows darn well whether it is dealing with table
> data or SQL commands.

Right, it does higher up in the code but I don't believe that knowledge
is passed down to the level it needs to be because it wasn't necessary
before and the module API didn't include a way to pass that information
into a given module.  I expect this is why the code currently depends on 
libpq to tell it when it has entered a 'copy in' state.  I'll look into 
this though and see just how invasive it would be to get the information 
to that level.

> Also, it might be possible to depend on whether libpq has entered the
> "copy in" state.  I'm not sure this works very well, because if there's
> an error in the COPY command itself (not in the following data) then we
> probably don't ever get into "copy in" state.

This is what the code currently depends on, and libpq (properly, imv)
doesn't enter the 'copy in' state when the COPY command itself fails.
That's exactly how we end up in this situation where pg_restore thinks
it's looking at a SQL command (because libpq said it wasn't in a COPY
state) when it's really getting COPY data from the higher level in the
code.

Thanks,

Stephen


signature.asc
Description: Digital signature