Re: [HACKERS] Beginner hacker item: Fix to_reg*() input type

[Petr Korobeinikov]

2016-01-05 21:04 GMT+03:00 Tom Lane :

> (I did make some small adjustments --- in this usage, PG_GETARG_TEXT_PP
> is good enough and likely a bit more efficient than PG_GETARG_TEXT_P.)
>
Also I forgot to bump catalog version up. My mishit.

[HACKERS] Add schema-qualified relnames in constraint error messages.

[Petr Korobeinikov]

Hackers,

At the moment schemas used as single-level namespaces.
It's quite convenient in large databases.

But error messages doesn’t contain schema names.

I have done a little patch with schema-qualified relation names in error
messages that produced by foreign key constraints and triggers.

Patch and usage example are attached.
Based on real bug hunting.

Pre-reviewed by Alexander Korotkov.
diff --git a/src/backend/utils/adt/ri_triggers.c 
b/src/backend/utils/adt/ri_triggers.c
index b476500..997e959 100644
--- a/src/backend/utils/adt/ri_triggers.c
+++ b/src/backend/utils/adt/ri_triggers.c
@@ -343,7 +343,7 @@ RI_FKey_check(TriggerData *trigdata)
ereport(ERROR,

(errcode(ERRCODE_FOREIGN_KEY_VIOLATION),
 errmsg("insert or 
update on table \"%s\" violates foreign key constraint \"%s\"",
-   
RelationGetRelationName(fk_rel),
+   
RelationGetNamespaceQualifiedRelationName(fk_rel),

NameStr(riinfo->conname)),
 errdetail("MATCH FULL 
does not allow mixing of null and nonnull key values."),
 
errtableconstraint(fk_rel,
@@ -2490,7 +2490,7 @@ RI_Initial_Check(Trigger *trigger, Relation fk_rel, 
Relation pk_rel)
ereport(ERROR,
(errcode(ERRCODE_FOREIGN_KEY_VIOLATION),
 errmsg("insert or update on table 
\"%s\" violates foreign key constraint \"%s\"",
-   
RelationGetRelationName(fk_rel),
+   
RelationGetNamespaceQualifiedRelationName(fk_rel),

NameStr(fake_riinfo.conname)),
 errdetail("MATCH FULL does not allow 
mixing of null and nonnull key values."),
 errtableconstraint(fk_rel,
@@ -2767,7 +2767,7 @@ ri_FetchConstraintInfo(Trigger *trigger, Relation 
trig_rel, bool rel_is_pk)
ereport(ERROR,
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
  errmsg("no pg_constraint entry for trigger \"%s\" on table 
\"%s\"",
-trigger->tgname, 
RelationGetRelationName(trig_rel)),
+trigger->tgname, 
RelationGetNamespaceQualifiedRelationName(trig_rel)),
 errhint("Remove this referential integrity 
trigger and its mates, then do ALTER TABLE ADD CONSTRAINT.")));
 
/* Find or create a hashtable entry for the constraint */
@@ -2779,14 +2779,14 @@ ri_FetchConstraintInfo(Trigger *trigger, Relation 
trig_rel, bool rel_is_pk)
if (riinfo->fk_relid != trigger->tgconstrrelid ||
riinfo->pk_relid != RelationGetRelid(trig_rel))
elog(ERROR, "wrong pg_constraint entry for trigger 
\"%s\" on table \"%s\"",
-trigger->tgname, 
RelationGetRelationName(trig_rel));
+trigger->tgname, 
RelationGetNamespaceQualifiedRelationName(trig_rel));
}
else
{
if (riinfo->fk_relid != RelationGetRelid(trig_rel) ||
riinfo->pk_relid != trigger->tgconstrrelid)
elog(ERROR, "wrong pg_constraint entry for trigger 
\"%s\" on table \"%s\"",
-trigger->tgname, 
RelationGetRelationName(trig_rel));
+trigger->tgname, 
RelationGetNamespaceQualifiedRelationName(trig_rel));
}
 
return riinfo;
@@ -3225,9 +3225,9 @@ ri_ReportViolation(const RI_ConstraintInfo *riinfo,
ereport(ERROR,
(errcode(ERRCODE_INTERNAL_ERROR),
 errmsg("referential integrity query on \"%s\" 
from constraint \"%s\" on \"%s\" gave unexpected result",
-   RelationGetRelationName(pk_rel),
+   
RelationGetNamespaceQualifiedRelationName(pk_rel),
NameStr(riinfo->conname),
-   
RelationGetRelationName(fk_rel)),
+   
RelationGetNamespaceQualifiedRelationName(fk_rel)),
 errhint("This is most likely due to a rule 
having rewritten the query.")));
 
/*
@@ -3315,28 +3315,28 @@ ri_ReportViolation(const RI_ConstraintInfo *riinfo,
 

Re: [HACKERS] Beginner hacker item: Fix to_reg*() input type

[Petr Korobeinikov]

> - Modify the functions in regproc.c. Take a look at how other text input
> functions work to see what needs to happen here (you'll want to use
> text_to_cstring() as part of that.)
>
> - Modify the appropriate entries in src/include/catalog/pg_proc.h

Let me try.
`make check` says "All 160 tests passed.".
diff --git a/src/backend/utils/adt/regproc.c b/src/backend/utils/adt/regproc.c
index 0f17eb8..eed1279 100644
--- a/src/backend/utils/adt/regproc.c
+++ b/src/backend/utils/adt/regproc.c
@@ -161,7 +161,7 @@ regprocin(PG_FUNCTION_ARGS)
 Datum
 to_regproc(PG_FUNCTION_ARGS)
 {
-   char   *pro_name = PG_GETARG_CSTRING(0);
+   const char *pro_name = text_to_cstring(PG_GETARG_TEXT_P(0));
List   *names;
FuncCandidateList clist;
 
@@ -331,7 +331,7 @@ regprocedurein(PG_FUNCTION_ARGS)
 Datum
 to_regprocedure(PG_FUNCTION_ARGS)
 {
-   char   *pro_name = PG_GETARG_CSTRING(0);
+   const char *pro_name = text_to_cstring(PG_GETARG_TEXT_P(0));
List   *names;
int nargs;
Oid argtypes[FUNC_MAX_ARGS];
@@ -620,7 +620,7 @@ regoperin(PG_FUNCTION_ARGS)
 Datum
 to_regoper(PG_FUNCTION_ARGS)
 {
-   char   *opr_name = PG_GETARG_CSTRING(0);
+   const char *opr_name = text_to_cstring(PG_GETARG_TEXT_P(0));
List   *names;
FuncCandidateList clist;
 
@@ -797,7 +797,7 @@ regoperatorin(PG_FUNCTION_ARGS)
 Datum
 to_regoperator(PG_FUNCTION_ARGS)
 {
-   char   *opr_name_or_oid = PG_GETARG_CSTRING(0);
+   const char *opr_name_or_oid = 
text_to_cstring(PG_GETARG_TEXT_P(0));
Oid result;
List   *names;
int nargs;
@@ -1061,7 +1061,7 @@ regclassin(PG_FUNCTION_ARGS)
 Datum
 to_regclass(PG_FUNCTION_ARGS)
 {
-   char   *class_name = PG_GETARG_CSTRING(0);
+   const char *class_name = text_to_cstring(PG_GETARG_TEXT_P(0));
Oid result;
List   *names;
 
@@ -1249,7 +1249,7 @@ regtypein(PG_FUNCTION_ARGS)
 Datum
 to_regtype(PG_FUNCTION_ARGS)
 {
-   char   *typ_name = PG_GETARG_CSTRING(0);
+   const char *typ_name = text_to_cstring(PG_GETARG_TEXT_P(0));
Oid result;
int32   typmod;
 
@@ -1606,7 +1606,7 @@ regrolein(PG_FUNCTION_ARGS)
 Datum
 to_regrole(PG_FUNCTION_ARGS)
 {
-   char   *role_name = PG_GETARG_CSTRING(0);
+   const char *role_name = text_to_cstring(PG_GETARG_TEXT_P(0));
Oid result;
List   *names;
 
@@ -1727,7 +1727,7 @@ regnamespacein(PG_FUNCTION_ARGS)
 Datum
 to_regnamespace(PG_FUNCTION_ARGS)
 {
-   char   *nsp_name = PG_GETARG_CSTRING(0);
+   const char *nsp_name = text_to_cstring(PG_GETARG_TEXT_P(0));
Oid result;
List   *names;
 
diff --git a/src/include/catalog/pg_proc.h b/src/include/catalog/pg_proc.h
index e5d6c77..22d9386 100644
--- a/src/include/catalog/pg_proc.h
+++ b/src/include/catalog/pg_proc.h
@@ -177,9 +177,9 @@ DATA(insert OID =  44 (  regprocin PGNSP PGUID 
12 1 0 0 0 f f f f t f s s 1
 DESCR("I/O");
 DATA(insert OID =  45 (  regprocout   PGNSP PGUID 12 1 0 0 0 f f f 
f t f s s 1 0 2275 "24" _null_ _null_ _null_ _null_ _null_ regprocout _null_ 
_null_ _null_ ));
 DESCR("I/O");
-DATA(insert OID = 3494 (  to_regproc   PGNSP PGUID 12 1 0 0 0 f f f f 
t f s s 1 0 24 "2275" _null_ _null_ _null_ _null_ _null_ to_regproc _null_ 
_null_ _null_ ));
+DATA(insert OID = 3494 (  to_regproc   PGNSP PGUID 12 1 0 0 0 f f f f 
t f s s 1 0 24 "25" _null_ _null_ _null_ _null_ _null_ to_regproc _null_ _null_ 
_null_ ));
 DESCR("convert proname to regproc");
-DATA(insert OID = 3479 (  to_regprocedure  PGNSP PGUID 12 1 0 0 0 f f f f 
t f s s 1 0 2202 "2275" _null_ _null_ _null_ _null_ _null_ to_regprocedure 
_null_ _null_ _null_ ));
+DATA(insert OID = 3479 (  to_regprocedure  PGNSP PGUID 12 1 0 0 0 f f f f 
t f s s 1 0 2202 "25" _null_ _null_ _null_ _null_ _null_ to_regprocedure _null_ 
_null_ _null_ ));
 DESCR("convert proname to regprocedure");
 DATA(insert OID =  46 (  textin   PGNSP PGUID 12 1 0 0 
0 f f f f t f i s 1 0 25 "2275" _null_ _null_ _null_ _null_ _null_ textin 
_null_ _null_ _null_ ));
 DESCR("I/O");
@@ -3483,9 +3483,9 @@ DATA(insert OID = 2214 (  regoperin   
PGNSP PGUID 12 1 0 0 0 f f f f t f s s 1 0
 DESCR("I/O");
 DATA(insert OID = 2215 (  regoperout   PGNSP PGUID 12 1 0 0 0 f f f f 
t f s s 1 0 2275 "2203" _null_ _null_ _null_ _null_ _null_ regoperout _null_ 
_null_ _null_ ));
 DESCR("I/O");
-DATA(insert OID = 3492 (  to_regoper   PGNSP PGUID 12 1 0 0 0 f f f f 
t f s s 1 0 2203 "2275" _null_ _null_ _null_ _null_ _null_ to_regoper _null_ 
_null_ _null_ ));
+DATA(insert OID = 3492 (  to_regoper   PGNSP PGUID 12 1 0 0 0 f f f f 
t f s 

Re: [HACKERS] Comfortably check BackendPID with psql

[Petr Korobeinikov]

+1 for Julien's patch.

Re: [HACKERS] psql :: support for \ev viewname and \sv viewname

[Petr Korobeinikov]

пт, 3 июля 2015 г. в 19:30, Tom Lane t...@sss.pgh.pa.us:

 So AFAICS, \ev and \sv should just number lines straightforwardly, with
 1 being the first line of the CREATE command text.  Am I missing
 something?


Fixed. Now both \ev and \sv  numbering lines starting with 1. New version
attached.

As I've already noticed that pg_get_viewdef() does not support full syntax
of creating or replacing views. In my opinion, psql source code isn't the
place where some formatting hacks should be. So, can you give me an idea
how to produce already formatted output supporting WITH statement without
breaking backward compatibility of pg_get_viewdef() internals?


psql-ev-sv-support-v6.diff
Description: Binary data

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

Re: [HACKERS] psql :: support for \ev viewname and \sv viewname

[Petr Korobeinikov]

 1.
 make failed with docs

Fixed.


 2.
  \ev vw1 3

 This syntax is supported. But documentation only says:
 \ev [ viewname ]
 Missing optional line_number clause

Fixed. Documented.

3.
  strip_lineno_from_objdesc(char *func)

 Can we have parameter name as obj instead of func.
 You have renamed the function name, as it is now called in case of views as
 well. Better rename the parameter names as well.

Renamed.

4.
 Also please update the comments above strip_lineno_from_objdesc().
 It is specific to functions which is not the case now.

 Comments updated.


 5.
  print_with_linenumbers(FILE *output,
   char *lines,
   const char *header_cmp_keyword,
   size_t header_cmp_sz)

 Can't we calculate the length of header (header_cmp_sz) inside function?
 This will avoid any sloppy changes like, change in the keyword but forgot
 to
 change the size.
 Lets just accept the keyword and calculate the size within the function.

Now header_cmp_sz calculated inside function.

6.
 *
 * Note that this loop scribbles on
 func_buf.
 */

 These lines at commands.c:1357, looks NO more valid now as there is NO loop
 there.

Removed.



 7.
 I see few comment lines explaining which is line 1 in case of function, for
 which AS  is used. Similarly, for view SELECT  is used.
 Can you add similar kind of explanation there?

Explanation added.

8.
  get_create_object_cmd_internal
  get_create_function_cmd
  get_create_view_cmd

 Can these three functions grouped together in just get_create_object_cmd().
 This function will take an extra parameter to indicate the object type.
 Say O_FUNC and O_VIEW for example.

 For distinct part, just have a switch case over this type.

 This will add a flexibility that if we add another such \e and \s options,
 we
 don't need new functions, rather just need new enum like O_new and a new
 case
 in this switch statement.
 Also it will look good to read the code as well.

 similarly you can do it for
  lookup_object_oid_internal
  get_create_function_cmd
  lookup_function_oid

Reworked.
New enum PgObjType introduced.



 9.
  static int count_lines_in_buf(PQExpBuffer buf)
  static void print_with_linenumbers(FILE *output, .. )
  static bool lookup_view_oid(const char *desc, Oid *view_oid)
  static bool lookup_object_oid_internal(PQExpBuffer query, Oid *obj_oid)

 Can we have smaller description, explaining what's the function doing for
 these functions at the definition?

Description added.



 10.
  + \\e, \\echo, \\ef, \\ev, \\encoding,

 Can you keep this sorted?
 It will be good if it sorted, but I see no such restriction as I see few
 out
 of order options. But better keep it ordered.
 Ignore if you dis-agree.

Hmm, sorted now.
Sort is based on my feelings.


psql-ev-sv-support-v4.diff
Description: Binary data

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

Re: [HACKERS] psql :: support for \ev viewname and \sv viewname

[Petr Korobeinikov]

Just a merge after pgindent run (807b9e0dff663c5da875af7907a5106c0ff90673).


psql-ev-sv-support-v3.diff
Description: Binary data

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

[HACKERS] psql :: support for \ev viewname and \sv viewname

[Petr Korobeinikov]

Hackers!

I'm proposing to add two new subcommands in psql:
1. \ev viewname - edit view definition with external editor (like \ef for
function)
2. \sv viewname - show view definition (like \sf for function, for
consistency)

What's inside:
1. review-ready implementation of \ev and \sv psql subcommands for editing
and viewing view's definition.
2. psql's doc update with new subcommands description.
3. a bit of extracting common source code parts into separate functions.
4. psql internal help update.
5. tab completion update.

There is one narrow place in this patch: current implementation doesn't
support create statement formatting for recursive views.

Any comments? Suggestions?
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 62a3b21..d668777 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -1651,6 +1651,28 @@ Tue Oct 26 21:40:57 CEST 1999
 
 
   varlistentry
+termliteral\ev optional replaceable 
class=parameterviewname/ /optional /literal/term
+
+listitem
+para
+ This command fetches and edits the definition of the named view,
+ in the form of a commandCREATE OR REPLACE VIEW/ command.
+ Editing is done in the same way as for literal\edit/.
+ After the editor exits, the updated command waits in the query buffer;
+ type semicolon or literal\g/ to send it, or literal\r/
+ to cancel.
+/para
+
+para
+ If no view is specified, a blank commandCREATE VEIW/
+ template is presented for editing.
+/para
+
+/listitem
+  /varlistentry
+
+
+  varlistentry
 termliteral\encoding [ replaceable 
class=parameterencoding/replaceable ]/literal/term
 
 listitem
@@ -2522,6 +2544,25 @@ testdb=gt; userinput\setenv LESS -imx4F/userinput
 
 
   varlistentry
+termliteral\sv[+] replaceable class=parameterviewname/ 
/literal/term
+
+listitem
+para
+ This command fetches and shows the definition of the named view,
+ in the form of a commandCREATE OR REPLACE VIEW/ command.
+ The definition is printed to the current query output channel,
+ as set by command\o/command.
+/para
+
+para
+ If literal+/literal is appended to the command name, then the
+ output lines are numbered, with the first line of the view definition
+ being line 1.
+/para
+  /varlistentry
+
+
+  varlistentry
 termliteral\t/literal/term
 listitem
 para
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 70b7d3b..948e381 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -60,9 +60,17 @@ static bool do_connect(char *dbname, char *user, char *host, 
char *port);
 static bool do_shell(const char *command);
 static bool do_watch(PQExpBuffer query_buf, long sleep);
 static bool lookup_function_oid(const char *desc, Oid *foid);
+static bool lookup_view_oid(const char *desc, Oid *view_oid);
 static bool get_create_function_cmd(Oid oid, PQExpBuffer buf);
-static int strip_lineno_from_funcdesc(char *func);
+static bool get_create_view_cmd(Oid oid, PQExpBuffer buf);
+static void format_create_view_cmd(char *view, PQExpBuffer buf);
+static int strip_lineno_from_objdesc(char *func);
 static void minimal_error_message(PGresult *res);
+static int count_lines_in_buf(PQExpBuffer buf);
+static void print_with_linenumbers(FILE *output,
+  char *lines,
+  const char 
*header_cmp_keyword,
+  size_t 
header_cmp_sz);
 
 static void printSSLInfo(void);
 static bool printPsetInfo(const char *param, struct printQueryOpt *popt);
@@ -612,7 +620,7 @@ exec_command(const char *cmd,
 
func = psql_scan_slash_option(scan_state,

  OT_WHOLE_LINE, NULL, true);
-   lineno = strip_lineno_from_funcdesc(func);
+   lineno = strip_lineno_from_objdesc(func);
if (lineno == 0)
{
/* error already reported */
@@ -682,6 +690,77 @@ exec_command(const char *cmd,
}
}
 
+   /*
+* \ev -- edit the named view, or present a blank CREATE VIEW viewname 
AS
+* template if no argument is given
+*/
+   else if (strcmp(cmd, ev) == 0)
+   {
+   int lineno = -1;
+
+   if (pset.sversion  70100)
+   {
+   psql_error(The server (version %d.%d) does not support 
editing view definition.\n,
+  pset.sversion / 1, 
(pset.sversion / 100) % 100);
+