Re: [PATCH] Provide rowcount for utility SELECTs - Mailing list pgsql-hackers

Boszormenyi Zoltan wrote:
> >>> Ah, I didn't even see that that section needed to be updated.  Good
> >>> catch.  I'd suggest the following wording:
> >>>
> >>> For a <command>SELECT</command> or <command>CREATE TABLE AS</command>
> >>> command, the tag is SELECT rows... [and the rest as you have it]
> >>>
> >>> We should probably also retitle that section from "Retrieving Result
> >>> Information for Other Commands" to "Retrieving Other Result
> >>> Information" and adjust the text of the opening sentence similarly.
> >>>
> >>> Also I think you need to update the docs for PQcmdtuples to mention
> >>> that it not works for SELECT and CTAS.
> >>>
> >>>
> >> Ok, I will update libpq.sgml where this section resides.
> >> What's a CTA, btw? Do you mean CTE, a.k.a. "Common Table Expression"?
> >>
> >
> > Sorry, CTAS = CREATE TABLE AS SELECT.
> >
>
> Okay, new patch is attached. Please read the docs changes, and comment.

I have reviewed this patch and made some adjustments, attached.  The
major change is that our return of "0 0" in certain cases must remain,
though I have improved the C comment explaining it with a separate CVS
commit:

    /*
     * If a command completion tag was supplied, use it.  Otherwise use the
     * portal's commandTag as the default completion tag.
     *
     * Exception: Clients expect INSERT/UPDATE/DELETE tags to have
     * counts, so fake them with zeros.  This can happen with DO INSTEAD
     * rules if there is no replacement query of the same type as the
     * original.  We print "0 0" here because technically there is no
     * query of the matching tag type, and printing a non-zero count for
     * a different query type seems wrong, e.g.  an INSERT that does
     * an UPDATE instead should not print "0 1" if one row
     * was updated.  See QueryRewrite(), step 3, for details.
     */

I have removed the part of the patch that chagned "0 0";  it seems to
run fine without it.  The rest of my adjustments were minor.

One major part of the patch seems to be the collection of the
PORTAL_ONE_SELECT switch label into the label below it, which is a nice
cleanup.

--
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  + If your life is a hard drive, Christ can be your backup. +
Index: doc/src/sgml/libpq.sgml
===================================================================
RCS file: /cvsroot/pgsql/doc/src/sgml/libpq.sgml,v
retrieving revision 1.297
diff -c -c -r1.297 libpq.sgml
*** doc/src/sgml/libpq.sgml    5 Feb 2010 03:09:04 -0000    1.297
--- doc/src/sgml/libpq.sgml    14 Feb 2010 03:11:00 -0000
***************
*** 2869,2880 ****
    </sect2>

    <sect2 id="libpq-exec-nonselect">
!    <title>Retrieving Result Information for Other Commands</title>

     <para>
!     These functions are used to extract information from
!     <structname>PGresult</structname> objects that are not
!     <command>SELECT</> results.
     </para>

     <variablelist>
--- 2869,2879 ----
    </sect2>

    <sect2 id="libpq-exec-nonselect">
!    <title>Retrieving Other Result Information</title>

     <para>
!     These functions are used to extract other information from
!     <structname>PGresult</structname> objects.
     </para>

     <variablelist>
***************
*** 2925,2936 ****
         This function returns a string containing the number of rows
         affected by the <acronym>SQL</> statement that generated the
         <structname>PGresult</>. This function can only be used following
!        the execution of an <command>INSERT</>, <command>UPDATE</>,
!        <command>DELETE</>, <command>MOVE</>, <command>FETCH</>, or
!        <command>COPY</> statement, or an <command>EXECUTE</> of a
!        prepared query that contains an <command>INSERT</>,
!        <command>UPDATE</>, or <command>DELETE</> statement.  If the
!        command that generated the <structname>PGresult</> was anything
         else, <function>PQcmdTuples</> returns an empty string. The caller
         should not free the return value directly. It will be freed when
         the associated <structname>PGresult</> handle is passed to
--- 2924,2935 ----
         This function returns a string containing the number of rows
         affected by the <acronym>SQL</> statement that generated the
         <structname>PGresult</>. This function can only be used following
!        the execution of a <command>SELECT</>, <command>CREATE TABLE AS</>,
!        <command>INSERT</>, <command>UPDATE</>, <command>DELETE</>,
!        <command>MOVE</>, <command>FETCH</>, or <command>COPY</> statement,
!        or an <command>EXECUTE</> of a prepared query that contains an
!        <command>INSERT</>, <command>UPDATE</>, or <command>DELETE</> statement.
!        If the command that generated the <structname>PGresult</> was anything
         else, <function>PQcmdTuples</> returns an empty string. The caller
         should not free the return value directly. It will be freed when
         the associated <structname>PGresult</> handle is passed to
Index: doc/src/sgml/protocol.sgml
===================================================================
RCS file: /cvsroot/pgsql/doc/src/sgml/protocol.sgml,v
retrieving revision 1.78
diff -c -c -r1.78 protocol.sgml
*** doc/src/sgml/protocol.sgml    3 Feb 2010 09:47:19 -0000    1.78
--- doc/src/sgml/protocol.sgml    14 Feb 2010 03:11:00 -0000
***************
*** 2222,2227 ****
--- 2222,2233 ----
         </para>

         <para>
+         For a <command>SELECT</command> or <command>CREATE TABLE AS</command>
+         command, the tag is <literal>SELECT <replaceable>rows</replaceable></literal>
+         where <replaceable>rows</replaceable> is the number of rows retrieved.
+        </para>
+
+        <para>
          For a <command>MOVE</command> command, the tag is
          <literal>MOVE <replaceable>rows</replaceable></literal> where
          <replaceable>rows</replaceable> is the number of rows the
Index: src/backend/tcop/pquery.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/tcop/pquery.c,v
retrieving revision 1.135
diff -c -c -r1.135 pquery.c
*** src/backend/tcop/pquery.c    13 Feb 2010 22:45:41 -0000    1.135
--- src/backend/tcop/pquery.c    14 Feb 2010 03:11:04 -0000
***************
*** 205,211 ****
          switch (queryDesc->operation)
          {
              case CMD_SELECT:
!                 strcpy(completionTag, "SELECT");
                  break;
              case CMD_INSERT:
                  if (queryDesc->estate->es_processed == 1)
--- 205,212 ----
          switch (queryDesc->operation)
          {
              case CMD_SELECT:
!                 snprintf(completionTag, COMPLETION_TAG_BUFSIZE,
!                          "SELECT %u", queryDesc->estate->es_processed);
                  break;
              case CMD_INSERT:
                  if (queryDesc->estate->es_processed == 1)
***************
*** 714,719 ****
--- 715,721 ----
            char *completionTag)
  {
      bool        result;
+     uint32        nprocessed;
      ResourceOwner saveTopTransactionResourceOwner;
      MemoryContext saveTopTransactionContext;
      Portal        saveActivePortal;
***************
*** 776,814 ****
          switch (portal->strategy)
          {
              case PORTAL_ONE_SELECT:
-                 (void) PortalRunSelect(portal, true, count, dest);
-
-                 /* we know the query is supposed to set the tag */
-                 if (completionTag && portal->commandTag)
-                     strcpy(completionTag, portal->commandTag);
-
-                 /* Mark portal not active */
-                 portal->status = PORTAL_READY;
-
-                 /*
-                  * Since it's a forward fetch, say DONE iff atEnd is now true.
-                  */
-                 result = portal->atEnd;
-                 break;
-
              case PORTAL_ONE_RETURNING:
              case PORTAL_UTIL_SELECT:

                  /*
                   * If we have not yet run the command, do so, storing its
!                  * results in the portal's tuplestore.
                   */
!                 if (!portal->holdStore)
                      FillPortalStore(portal, isTopLevel);

                  /*
                   * Now fetch desired portion of results.
                   */
!                 (void) PortalRunSelect(portal, true, count, dest);

!                 /* we know the query is supposed to set the tag */
                  if (completionTag && portal->commandTag)
!                     strcpy(completionTag, portal->commandTag);

                  /* Mark portal not active */
                  portal->status = PORTAL_READY;
--- 778,812 ----
          switch (portal->strategy)
          {
              case PORTAL_ONE_SELECT:
              case PORTAL_ONE_RETURNING:
              case PORTAL_UTIL_SELECT:

                  /*
                   * If we have not yet run the command, do so, storing its
!                  * results in the portal's tuplestore. Do this only for the
!                  * PORTAL_ONE_RETURNING and PORTAL_UTIL_SELECT cases.
                   */
!                 if (portal->strategy != PORTAL_ONE_SELECT && !portal->holdStore)
                      FillPortalStore(portal, isTopLevel);

                  /*
                   * Now fetch desired portion of results.
                   */
!                 nprocessed = PortalRunSelect(portal, true, count, dest);

!                 /*
!                  * If the portal result contains a command tag and the caller
!                  * gave us a pointer to store it, copy it. Patch the "SELECT"
!                  * tag to also provide the rowcount.
!                  */
                  if (completionTag && portal->commandTag)
!                 {
!                     if (strcmp(portal->commandTag, "SELECT") == 0)
!                         snprintf(completionTag, COMPLETION_TAG_BUFSIZE,
!                                         "SELECT %u", nprocessed);
!                     else
!                         strcpy(completionTag, portal->commandTag);
!                 }

                  /* Mark portal not active */
                  portal->status = PORTAL_READY;
***************
*** 1331,1337 ****
      {
          if (portal->commandTag)
              strcpy(completionTag, portal->commandTag);
!         if (strcmp(completionTag, "INSERT") == 0)
              strcpy(completionTag, "INSERT 0 0");
          else if (strcmp(completionTag, "UPDATE") == 0)
              strcpy(completionTag, "UPDATE 0");
--- 1329,1337 ----
      {
          if (portal->commandTag)
              strcpy(completionTag, portal->commandTag);
!         if (strcmp(completionTag, "SELECT") == 0)
!             sprintf(completionTag, "SELECT 0 0");
!         else if (strcmp(completionTag, "INSERT") == 0)
              strcpy(completionTag, "INSERT 0 0");
          else if (strcmp(completionTag, "UPDATE") == 0)
              strcpy(completionTag, "UPDATE 0");
Index: src/interfaces/libpq/fe-exec.c
===================================================================
RCS file: /cvsroot/pgsql/src/interfaces/libpq/fe-exec.c,v
retrieving revision 1.208
diff -c -c -r1.208 fe-exec.c
*** src/interfaces/libpq/fe-exec.c    21 Jan 2010 18:43:25 -0000    1.208
--- src/interfaces/libpq/fe-exec.c    14 Feb 2010 03:11:04 -0000
***************
*** 2752,2758 ****
              goto interpret_error;        /* no space? */
          p++;
      }
!     else if (strncmp(res->cmdStatus, "DELETE ", 7) == 0 ||
               strncmp(res->cmdStatus, "UPDATE ", 7) == 0)
          p = res->cmdStatus + 7;
      else if (strncmp(res->cmdStatus, "FETCH ", 6) == 0)
--- 2752,2759 ----
              goto interpret_error;        /* no space? */
          p++;
      }
!     else if (strncmp(res->cmdStatus, "SELECT ", 7) == 0 ||
!              strncmp(res->cmdStatus, "DELETE ", 7) == 0 ||
               strncmp(res->cmdStatus, "UPDATE ", 7) == 0)
          p = res->cmdStatus + 7;
      else if (strncmp(res->cmdStatus, "FETCH ", 6) == 0)