[PATCHES] pg_restore COPY error handling

[Stephen Frost]

* Stephen Frost ([EMAIL PROTECTED]) wrote:
 Needs to be changed to handle whitespace in front of the actual 'COPY',
 unless someone else has a better idea.  This should be reasonably
 trivial to do though...  If you'd like me to make that change and send
 in a new patch, just let me know.

Fixed, using isspace().  Also added an output message to make it a bit
clearer when a failed COPY has been detected, etc.

Updated patch attached.

Thanks,

Stephen
Index: src/bin/pg_dump/pg_backup_db.c
===
RCS file: /projects/cvsroot/pgsql/src/bin/pg_dump/pg_backup_db.c,v
retrieving revision 1.66
diff -c -r1.66 pg_backup_db.c
*** src/bin/pg_dump/pg_backup_db.c  15 Oct 2005 02:49:38 -  1.66
--- src/bin/pg_dump/pg_backup_db.c  21 Jan 2006 19:54:56 -
***
*** 292,297 
--- 292,298 
PGconn *conn = AH-connection;
PGresult   *res;
charerrStmt[DB_MAX_ERR_STMT];
+   int wsoffset = 0;
  
/* fprintf(stderr, Executing: '%s'\n\n, qry-data); */
res = PQexec(conn, qry-data);
***
*** 306,311 
--- 307,317 
}
else
{
+   /* Catch that this is a failed copy command, and
+* set pgCopyIn accordingly */
+   while (isspace(qry-data[wsoffset])) wsoffset++;
+   if (strncasecmp(qry-data+wsoffset,COPY ,5) == 0) 
AH-pgCopyIn = -1;
+ 
strncpy(errStmt, qry-data, DB_MAX_ERR_STMT);
if (errStmt[DB_MAX_ERR_STMT - 1] != '\0')
{
***
*** 317,322 
--- 323,330 
warn_or_die_horribly(AH, modulename, %s: %sCommand 
was: %s\n,
 desc, 
PQerrorMessage(AH-connection),
 errStmt);
+ 
+   if (AH-pgCopyIn == -1) write_msg(NULL, COPY failed, 
skipping COPY data.\n);
}
}
  
***
*** 389,395 
 *-
 */
  
!   if (PQputline(AH-connection, AH-pgCopyBuf-data) != 0)
die_horribly(AH, modulename, error returned by PQputline\n);
  
resetPQExpBuffer(AH-pgCopyBuf);
--- 397,405 
 *-
 */
  
!   /* If this is a failed copy command (pgCopyIn == -1) then just
!* fall through */
!   if (AH-pgCopyIn == 1  PQputline(AH-connection, AH-pgCopyBuf-data) 
!= 0)
die_horribly(AH, modulename, error returned by PQputline\n);
  
resetPQExpBuffer(AH-pgCopyBuf);
***
*** 400,406 
  
if (isEnd)
{
!   if (PQendcopy(AH-connection) != 0)
die_horribly(AH, modulename, error returned by 
PQendcopy\n);
  
AH-pgCopyIn = 0;
--- 410,418 
  
if (isEnd)
{
!   /* If this is a failed copy command (pgCopyIn == -1) then just
!* fall through */
!   if (AH-pgCopyIn == 1  PQendcopy(AH-connection) != 0)
die_horribly(AH, modulename, error returned by 
PQendcopy\n);
  
AH-pgCopyIn = 0;
***
*** 615,621 
/* Could switch between command and COPY IN mode at each line */
while (qry  eos)
{
!   if (AH-pgCopyIn)
qry = _sendCopyLine(AH, qry, eos);
else
qry = _sendSQLLine(AH, qry, eos);
--- 627,637 
/* Could switch between command and COPY IN mode at each line */
while (qry  eos)
{
!   /* If this is a working COPY *or* a failed COPY, call
!* _sendCopyLine to handle the incoming data from the COPY
!* command, it will just circular-file the data if we're
!* running a failed COPY. */
!   if (AH-pgCopyIn == 1 || AH-pgCopyIn == -1)
qry = _sendCopyLine(AH, qry, eos);
else
qry = _sendSQLLine(AH, qry, eos);

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

   http://archives.postgresql.org

[PATCHES] postmaster/postgres merge for testing

[Peter Eisentraut]

Attached is a patch that merges postmaster and postgres into just a 
postmaster command.  I have moved a few things around so it would be 
good if someone could test this especially on Windows (just building 
and regression test should do it).

(It's a bit weird in that src/backend already contains a postmaster 
directory so you can't build a postmaster file there.  So in the 
build tree it's called postmaster_.  Feel free to make better 
suggestions.)

-- 
Peter Eisentraut
http://developer.postgresql.org/~petere/


postmaster-merge.patch.bz2
Description: BZip2 compressed data

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

Re: [PATCHES] postmaster/postgres merge for testing

[Tom Lane]

Peter Eisentraut [EMAIL PROTECTED] writes:
 (It's a bit weird in that src/backend already contains a postmaster 
 directory so you can't build a postmaster file there.  So in the 
 build tree it's called postmaster_.  Feel free to make better 
 suggestions.)

backend, maybe?  Or just keep calling it postgres at that point.

regards, tom lane

---(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] Allow an alias for the target table in UPDATE/DELETE

[Neil Conway]

On Sat, 2005-12-03 at 10:42 +0900, Atsushi Ogawa wrote:
 Thanks for comments. I modified the patch.

Patch applied to HEAD.

From looking at SQL2003, it seems to me that this syntax is actually
specified by the standard:

update statement: searched ::=
UPDATE target table [ [ AS ] correlation name ]
SET set clause list
[ WHERE search condition ]

delete statement: searched ::=
DELETE FROM target table [ [ AS ] correlation name ]
[ WHERE search condition ]

I think we ought to support using the alias in the SET clause,
particularly as the standard allows for it (AFAIK).

-Neil



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

Re: [PATCHES] Allow an alias for the target table in UPDATE/DELETE

[Tom Lane]

Neil Conway [EMAIL PROTECTED] writes:
 From looking at SQL2003, it seems to me that this syntax is actually
 specified by the standard:

 update statement: searched ::=
 UPDATE target table [ [ AS ] correlation name ]
 SET set clause list
 [ WHERE search condition ]

 delete statement: searched ::=
 DELETE FROM target table [ [ AS ] correlation name ]
 [ WHERE search condition ]

Interesting, because the AS clause is most definitely *not* in
SQL92 or SQL99.

I'm all for this change, in any case, but it's interesting to notice
that the SQL committee actually does respond to the masses ...

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] TupleDesc refcounting

[Neil Conway]

On Thu, 2006-01-19 at 04:14 -0500, Neil Conway wrote:
 The patch is WIP: a few regression tests fail due to TupleDesc leaks I
 haven't fixed yet but that should be easily fixable, and there are a few
 other minor issues to address. I'm just posting the patch now to get any
 feedback.

Attached is a revised patch: all the regression tests patch, and I'm not
aware of any major remaining issues. Barring any objections, I'll apply
this tomorrow.

(I'll take a look at using refcounting for TupleDescs in other parts of
the system after that...)

-Neil


*** src/backend/access/common/tupdesc.c	83ca807d4fdd572c409bc9214922b6ba9da7ce18
--- src/backend/access/common/tupdesc.c	1a84f6faf6fc647c2bad0c2381f976f65b703f59
***
*** 23,28 
--- 23,29 
  #include catalog/pg_type.h
  #include parser/parse_type.h
  #include utils/builtins.h
+ #include utils/resowner.h
  #include utils/syscache.h
  
  
***
*** 84,89 
--- 85,91 
  	desc-tdtypeid = RECORDOID;
  	desc-tdtypmod = -1;
  	desc-tdhasoid = hasoid;
+ 	desc-refcount = 0;
  
  	return desc;
  }
***
*** 116,121 
--- 118,124 
  	desc-tdtypeid = RECORDOID;
  	desc-tdtypmod = -1;
  	desc-tdhasoid = hasoid;
+ 	desc-refcount = 0;
  
  	return desc;
  }
***
*** 214,219 
--- 217,224 
  {
  	int			i;
  
+ 	Assert(tupdesc-refcount == 0);
+ 
  	if (tupdesc-constr)
  	{
  		if (tupdesc-constr-num_defval  0)
***
*** 246,251 
--- 251,277 
  	pfree(tupdesc);
  }
  
+ void
+ IncrTupleDescRefCount(TupleDesc tupdesc)
+ {
+ 	Assert(tupdesc-refcount = 0);
+ 
+ 	ResourceOwnerEnlargeTupleDescs(CurrentResourceOwner);
+ 	tupdesc-refcount++;
+ 	ResourceOwnerRememberTupleDesc(CurrentResourceOwner, tupdesc);
+ }
+ 
+ void
+ DecrTupleDescRefCount(TupleDesc tupdesc)
+ {
+ 	Assert(tupdesc-refcount  0);
+ 
+ 	tupdesc-refcount--;
+ 	ResourceOwnerForgetTupleDesc(CurrentResourceOwner, tupdesc);
+ 	if (tupdesc-refcount == 0)
+ 		FreeTupleDesc(tupdesc);
+ }
+ 
  /*
   * Compare two TupleDesc structures for logical equality
   *

*** src/backend/access/heap/tuptoaster.c	f3ec822c0e6b6328bc0026716fcf4b133f89b092
--- src/backend/access/heap/tuptoaster.c	bbdea6bdd98d250326fad68899d07a8b153fb263
***
*** 892,898 
--- 892,901 
  	 * If nothing to untoast, just return the original tuple.
  	 */
  	if (!need_change)
+ 	{
+ 		DecrTupleDescRefCount(tupleDesc);
  		return value;
+ 	}
  
  	/*
  	 * Calculate the new size of the tuple.  Header size should not change,
***
*** 930,935 
--- 933,939 
  		if (toast_free[i])
  			pfree(DatumGetPointer(toast_values[i]));
  
+ 	DecrTupleDescRefCount(tupleDesc);
  	return PointerGetDatum(new_data);
  }
  

*** src/backend/executor/execQual.c	eb1daef85341439578a3f6bb73549c4420292bc3
--- src/backend/executor/execQual.c	00e68d0f9dc2b692b994608acc001f640679f7a8
***
*** 94,99 
--- 94,100 
  static Datum ExecEvalConvertRowtype(ConvertRowtypeExprState *cstate,
  	   ExprContext *econtext,
  	   bool *isNull, ExprDoneCond *isDone);
+ static void CleanupConvertRowtypeCallback(Datum arg);
  static Datum ExecEvalCase(CaseExprState *caseExpr, ExprContext *econtext,
  			 bool *isNull, ExprDoneCond *isDone);
  static Datum ExecEvalCaseTestExpr(ExprState *exprstate,
***
*** 132,140 
--- 133,143 
  static Datum ExecEvalFieldSelect(FieldSelectState *fstate,
  	ExprContext *econtext,
  	bool *isNull, ExprDoneCond *isDone);
+ static void CleanupFieldSelectCallback(Datum arg);
  static Datum ExecEvalFieldStore(FieldStoreState *fstate,
     ExprContext *econtext,
     bool *isNull, ExprDoneCond *isDone);
+ static void CleanupFieldStoreCallback(Datum arg);
  static Datum ExecEvalRelabelType(GenericExprState *exprstate,
  	ExprContext *econtext,
  	bool *isNull, ExprDoneCond *isDone);
***
*** 707,712 
--- 710,717 
  		  attrno,
  		  tupDesc,
  		  isNull);
+ 
+ 	DecrTupleDescRefCount(tupDesc);
  	return result;
  }
  
***
*** 765,770 
--- 770,777 
  		  attrno,
  		  tupDesc,
  		  isNull);
+ 
+ 	DecrTupleDescRefCount(tupDesc);
  	return result;
  }
  
***
*** 1149,1157 
  /*
   *		ExecMakeTableFunctionResult
   *
!  * Evaluate a table function, producing a materialized result in a Tuplestore
!  * object.	*returnDesc is set to the tupledesc actually returned by the
!  * function, or NULL if it didn't provide one.
   */
  Tuplestorestate *
  ExecMakeTableFunctionResult(ExprState *funcexpr,
--- 1156,1166 
  /*
   *		ExecMakeTableFunctionResult
   *
!  * Evaluate a table function, producing a materialized result in a
!  * Tuplestore object. *returnDesc is set to the tupledesc actually
!  * returned by the function, or NULL if it didn't 

Re: [PATCHES] Allow an alias for the target table in UPDATE/DELETE

[Tom Lane]

Neil Conway [EMAIL PROTECTED] writes:
 Patch applied to HEAD.

This is wrong:

+relation_expr_opt_alias: relation_expr
+   {
+   $$ = $1;
+   }
+   | relation_expr opt_as IDENT
+   {
+

Should be ColId, not IDENT, per existing alias_clause production.

Also, while I'm all for getting to 100 regression tests, this is a
mighty lame 100th entry.

regards, tom lane

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

Re: [PATCHES] Allow an alias for the target table in UPDATE/DELETE

[Neil Conway]

On Sun, 2006-01-22 at 00:55 -0500, Tom Lane wrote:
 This is wrong:
 
 +relation_expr_opt_alias: relation_expr
 + {
 + $$ = $1;
 + }
 + | relation_expr opt_as IDENT
 + {
 +
 
 Should be ColId, not IDENT, per existing alias_clause production.

That causes a reduce/reduce conflict:

state 557

  934 relation_expr_opt_alias: relation_expr .
  935| relation_expr . opt_as ColId

AS  shift, and go to state 875

$end  reduce using rule 934 (relation_expr_opt_alias)
SET   reduce using rule 754 (opt_as)
SET   [reduce using rule 934 (relation_expr_opt_alias)]
USING reduce using rule 934 (relation_expr_opt_alias)
WHERE reduce using rule 934 (relation_expr_opt_alias)
')'   reduce using rule 934 (relation_expr_opt_alias)
';'   reduce using rule 934 (relation_expr_opt_alias)
$default  reduce using rule 754 (opt_as)

opt_as  go to state 876

 Also, while I'm all for getting to 100 regression tests, this is a
 mighty lame 100th entry.

Why's that? We needed regression tests for the changes to DELETE (IMHO),
and I didn't see an existing test file where it would have made much
sense to add them. I don't think the barrier for adding a new regression
test should be particularly high, provided the test covers a clear set
of functionality (such as the DELETE statement).

-Neil



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

Re: [PATCHES] Allow an alias for the target table in UPDATE/DELETE

[Tom Lane]

Neil Conway [EMAIL PROTECTED] writes:
 On Sun, 2006-01-22 at 00:55 -0500, Tom Lane wrote:
 Should be ColId, not IDENT, per existing alias_clause production.

 That causes a reduce/reduce conflict:

The grammar change is the one marginally nontrivial part of the patch,
and you couldn't be bothered to get it right?  Try harder.

regards, tom lane

---(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] Allow an alias for the target table in UPDATE/DELETE

[Neil Conway]

On Sun, 2006-01-22 at 01:28 -0500, Tom Lane wrote:
 The grammar change is the one marginally nontrivial part of the patch,
 and you couldn't be bothered to get it right?  Try harder.

:-(

I believe the conflict results from the fact that ColId can include SET
(since it is an unreserved_keyword), but SET might also be the next
token in the UpdateStmt, and yacc is not capable of distinguishing
between these two cases. We could fix this by promoting SET to be a
func_name_keyword or reserved_keyword, but that seems unfortunate. Can
you see a better fix?

-Neil



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

Re: [PATCHES] Allow an alias for the target table in UPDATE/DELETE

[Tom Lane]

Neil Conway [EMAIL PROTECTED] writes:
 Can you see a better fix?

I haven't done any experimentation, but my first instinct would be to
spell out the productions at greater length: instead of 

relation_expr opt_as ColId

try

relation_expr ColId
| relation_expr AS ColId

The normal game with bison is to postpone decisions (reductions) as
long as possible.  Shortcuts like opt_as lose that game because the
shift-versus-reduce decision has to be made with hardly any lookahead.

Or maybe some other hack is needed, but I seriously doubt it's
unfixable.

regards, tom lane

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

Re: [PATCHES] Allow an alias for the target table in UPDATE/DELETE

[Neil Conway]

On Sun, 2006-01-22 at 02:05 -0500, Neil Conway wrote:
 I believe the conflict results from the fact that ColId can include SET
 (since it is an unreserved_keyword), but SET might also be the next
 token in the UpdateStmt, and yacc is not capable of distinguishing
 between these two cases. We could fix this by promoting SET to be a
 func_name_keyword or reserved_keyword, but that seems unfortunate.

On thinking about this a bit more, an alternative fix would be to make
AS mandatory. That is unappealing because the SQL spec requires that it
be optional, as well as for consistency with aliases in SELECT's FROM
list.

Another possibility is to disallow SET here, but not in other places
where ColId is used. That is, some hack like:

ColId_no_set: IDENT | unreserved_keyword | col_name_keyword ;
ColId: ColId_no_set | SET ;

relation_expr_opt_alias: relation_expr
| relation_expr opt_as ColId_no_set

... along the corresponding changes to the other productions that deal
with keywords (removing SET from unreserved_keywords and adding it
manually as an alternative to type_name, function_name, etc.). Needless
to say, that is also very ugly.

-Neil



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

   http://archives.postgresql.org

Re: [PATCHES] Allow an alias for the target table in UPDATE/DELETE

[Neil Conway]

On Sun, 2006-01-22 at 02:23 -0500, Tom Lane wrote:
   relation_expr opt_as ColId
 
 try
 
   relation_expr ColId
   | relation_expr AS ColId

Yeah, I already tried that without success. The no-AS-specified variant
is still ambiguous: given SET following relation_expr, the parser still
can't determine whether the SET is the table alias or is part of the
UpdateStmt.

-Neil



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