($INBOX_DIR/description missing)  
help / color / mirror / Atom feed
[PATCH] Clean up pg_checksums.sgml
21+ messages / 5 participants
[nested] [flat]

* [PATCH] Clean up pg_checksums.sgml
@ 2019-03-29 00:20  Justin Pryzby <[email protected]>
  0 siblings, 0 replies; 21+ messages in thread

From: Justin Pryzby @ 2019-03-29 00:20 UTC (permalink / raw)

---
 doc/src/sgml/ref/pg_checksums.sgml | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/doc/src/sgml/ref/pg_checksums.sgml b/doc/src/sgml/ref/pg_checksums.sgml
index 47d4a62..01c65fe 100644
--- a/doc/src/sgml/ref/pg_checksums.sgml
+++ b/doc/src/sgml/ref/pg_checksums.sgml
@@ -39,15 +39,16 @@ PostgreSQL documentation
    <application>pg_checksums</application> checks, enables or disables data
    checksums in a <productname>PostgreSQL</productname> cluster.  The server
    must be shut down cleanly before running
-   <application>pg_checksums</application>. The exit status is zero if there
-   are no checksum errors when checking them, and nonzero if at least one
-   checksum failure is detected. If enabling or disabling checksums, the
+   <application>pg_checksums</application>. When verifying checksums, the exit
+   status is zero if there are no checksum errors, and nonzero if at least one
+   checksum failure is detected. When enabling or disabling checksums, the
    exit status is nonzero if the operation failed.
   </para>
 
   <para>
-   While checking or enabling checksums needs to scan or write every file in
-   the cluster, disabling checksums will only update the file
+   When verifying checksums, every file in the cluster is scanned;
+   When enabling checksums, every file in the cluster is also rewritten.
+   Disabling checksums only updates the file
    <filename>pg_control</filename>.
   </para>
  </refsect1>
@@ -218,10 +219,9 @@ PostgreSQL documentation
   </para>
   <para>
    If <application>pg_checksums</application> is aborted or killed while
-   enabling or disabling checksums, the cluster will keep the same
-   configuration for data checksums as before the operation attempted.
-   <application>pg_checksums</application> can be restarted to
-   attempt again the same operation.
+   enabling or disabling checksums, the cluster's checksum state will be
+   unchanged, and <application>pg_checksums</application> would need to be
+   rerun and start its operation from scratch.
   </para>
  </refsect1>
 </refentry>
-- 
2.1.4


--WYTEVAkct0FjGQmd--





^ permalink  raw  reply  [nested|flat] 21+ messages in thread

* [PATCH] Clean up pg_checksums.sgml
@ 2019-03-29 00:20  Justin Pryzby <[email protected]>
  0 siblings, 0 replies; 21+ messages in thread

From: Justin Pryzby @ 2019-03-29 00:20 UTC (permalink / raw)

---
 doc/src/sgml/ref/pg_checksums.sgml | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/doc/src/sgml/ref/pg_checksums.sgml b/doc/src/sgml/ref/pg_checksums.sgml
index c60695d..9433782 100644
--- a/doc/src/sgml/ref/pg_checksums.sgml
+++ b/doc/src/sgml/ref/pg_checksums.sgml
@@ -39,15 +39,16 @@ PostgreSQL documentation
    <application>pg_checksums</application> verifies, enables or disables data
    checksums in a <productname>PostgreSQL</productname> cluster.  The server
    must be shut down cleanly before running
-   <application>pg_checksums</application>. The exit status is zero if there
-   are no checksum errors when checking them, and nonzero if at least one
-   checksum failure is detected. If enabling or disabling checksums, the
+   <application>pg_checksums</application>. When verifying checksums, the exit
+   status is zero if there are no checksum errors, and nonzero if at least one
+   checksum failure is detected. When enabling or disabling checksums, the
    exit status is nonzero if the operation failed.
   </para>
 
   <para>
-   While verifying or enabling checksums needs to scan or write every file in
-   the cluster, disabling checksums will only update the file
+   When verifying checksums, every file in the cluster is scanned;
+   When enabling checksums, every file in the cluster is also rewritten.
+   Disabling checksums only updates the file
    <filename>pg_control</filename>.
   </para>
  </refsect1>
@@ -195,11 +196,10 @@ PostgreSQL documentation
    safe.
   </para>
   <para>
-   If <application>pg_checksums</application> is aborted or killed while
-   enabling or disabling checksums, the cluster will keep the same
-   configuration for data checksums as before the operation attempted.
-   <application>pg_checksums</application> can be restarted to
-   attempt again the same operation.
+   If <application>pg_checksums</application> is aborted or killed
+   while enabling or disabling checksums, the cluster's checksum state
+   will be unchanged, and <application>pg_checksums</application> would need to
+   be rerun and start its operation from scratch.
   </para>
  </refsect1>
 </refentry>
-- 
2.1.4


--wq9mPyueHGvFACwf--





^ permalink  raw  reply  [nested|flat] 21+ messages in thread

* [PATCH v2 12/12] docs: create or reindex progress of ptn tables
@ 2019-04-05 00:39  Justin Pryzby <[email protected]>
  0 siblings, 0 replies; 21+ messages in thread

From: Justin Pryzby @ 2019-04-05 00:39 UTC (permalink / raw)

---
 doc/src/sgml/monitoring.sgml | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index a1c3848..5ada8b7 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -3582,16 +3582,16 @@ SELECT pg_stat_get_backend_pid(s.backendid) AS pid,
       <entry><structfield>partitions_total</structfield></entry>
       <entry><type>bigint</type></entry>
       <entry>
-       When creating an index on a partitioned table, this column is set to
-       the total number of partitions on which the index is to be created.
+       When the index being created or reindexed is on a partitioned table,
+       this is the total number of partitions to be processed.
       </entry>
      </row>
      <row>
       <entry><structfield>partitions_done</structfield></entry>
       <entry><type>bigint</type></entry>
       <entry>
-       When creating an index on a partitioned table, this column is set to
-       the number of partitions on which the index has been completed.
+       When the index being created or reindexed is on a partitioned table,
+       this is the number of partitions for which the process is complete.
       </entry>
      </row>
     </tbody>
-- 
2.1.4


--u65IjBhB3TIa72Vp--





^ permalink  raw  reply  [nested|flat] 21+ messages in thread

* [PATCH v2 1/1] add timing information to pg_upgrade
@ 2023-07-27 23:16  Nathan Bossart <[email protected]>
  0 siblings, 0 replies; 21+ messages in thread

From: Nathan Bossart @ 2023-07-27 23:16 UTC (permalink / raw)

---
 src/bin/pg_upgrade/util.c | 55 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 54 insertions(+), 1 deletion(-)

diff --git a/src/bin/pg_upgrade/util.c b/src/bin/pg_upgrade/util.c
index 21ba4c8f12..25e43a593a 100644
--- a/src/bin/pg_upgrade/util.c
+++ b/src/bin/pg_upgrade/util.c
@@ -9,14 +9,19 @@
 
 #include "postgres_fe.h"
 
+#include <math.h>
 #include <signal.h>
 
 #include "common/username.h"
 #include "pg_upgrade.h"
+#include "portability/instr_time.h"
 
 LogOpts		log_opts;
 
+static instr_time step_start;
+
 static void pg_log_v(eLogType type, const char *fmt, va_list ap) pg_attribute_printf(2, 0);
+static char *get_time_str(double time_ms);
 
 
 /*
@@ -137,6 +142,8 @@ prep_status(const char *fmt,...)
 
 	/* trim strings */
 	pg_log(PG_REPORT_NONL, "%-*s", MESSAGE_WIDTH, message);
+
+	INSTR_TIME_SET_CURRENT(step_start);
 }
 
 /*
@@ -170,6 +177,8 @@ prep_status_progress(const char *fmt,...)
 		pg_log(PG_REPORT, "%-*s", MESSAGE_WIDTH, message);
 	else
 		pg_log(PG_REPORT_NONL, "%-*s", MESSAGE_WIDTH, message);
+
+	INSTR_TIME_SET_CURRENT(step_start);
 }
 
 static void
@@ -280,11 +289,55 @@ pg_fatal(const char *fmt,...)
 }
 
 
+static char *
+get_time_str(double time_ms)
+{
+	double		seconds;
+	double		minutes;
+	double		hours;
+	double		days;
+
+	if (time_ms < 1000.0)
+		return psprintf("%.3f ms", time_ms);
+
+	seconds = time_ms / 1000.0;
+	minutes = floor(seconds / 60.0);
+	seconds -= 60.0 * minutes;
+	if (minutes < 60.0)
+		return psprintf("%.3f ms (%02d:%06.3f)",
+						time_ms, (int) minutes, seconds);
+
+	hours = floor(minutes / 60.0);
+	minutes -= 60.0 * hours;
+	if (hours < 24.0)
+		return psprintf("%.3f ms (%02d:%02d:%06.3f)",
+						time_ms, (int) hours, (int) minutes, seconds);
+
+	days = floor(hours / 24.0);
+	hours -= 24.0 * days;
+	return psprintf("%.3f ms (%.0f d %02d:%02d:%06.3f)",
+					time_ms, days, (int) hours, (int) minutes, seconds);
+}
+
+
 void
 check_ok(void)
 {
+	instr_time	step_end;
+	char	   *step_time;
+
+	Assert(!INSTR_TIME_IS_ZERO(step_start));
+
+	INSTR_TIME_SET_CURRENT(step_end);
+	INSTR_TIME_SUBTRACT(step_end, step_start);
+	step_time = get_time_str(INSTR_TIME_GET_MILLISEC(step_end));
+
+	/* reset start time */
+	INSTR_TIME_SET_ZERO(step_start);
+
 	/* all seems well */
-	report_status(PG_REPORT, "ok");
+	report_status(PG_REPORT, "ok\t%s", step_time);
+	pfree(step_time);
 }
 
 
-- 
2.25.1


--vtzGhvizbBRQ85DL--





^ permalink  raw  reply  [nested|flat] 21+ messages in thread

* [PATCH v3 2/2] add timing information to pg_upgrade
@ 2023-07-27 23:16  Nathan Bossart <[email protected]>
  0 siblings, 0 replies; 21+ messages in thread

From: Nathan Bossart @ 2023-07-27 23:16 UTC (permalink / raw)

---
 src/bin/pg_upgrade/util.c | 55 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 54 insertions(+), 1 deletion(-)

diff --git a/src/bin/pg_upgrade/util.c b/src/bin/pg_upgrade/util.c
index 21ba4c8f12..23fd5f87af 100644
--- a/src/bin/pg_upgrade/util.c
+++ b/src/bin/pg_upgrade/util.c
@@ -9,14 +9,19 @@
 
 #include "postgres_fe.h"
 
+#include <math.h>
 #include <signal.h>
 
 #include "common/username.h"
 #include "pg_upgrade.h"
+#include "portability/instr_time.h"
 
 LogOpts		log_opts;
 
+static instr_time step_start;
+
 static void pg_log_v(eLogType type, const char *fmt, va_list ap) pg_attribute_printf(2, 0);
+static char *get_time_str(double time_ms);
 
 
 /*
@@ -137,6 +142,8 @@ prep_status(const char *fmt,...)
 
 	/* trim strings */
 	pg_log(PG_REPORT_NONL, "%-*s", MESSAGE_WIDTH, message);
+
+	INSTR_TIME_SET_CURRENT(step_start);
 }
 
 /*
@@ -170,6 +177,8 @@ prep_status_progress(const char *fmt,...)
 		pg_log(PG_REPORT, "%-*s", MESSAGE_WIDTH, message);
 	else
 		pg_log(PG_REPORT_NONL, "%-*s", MESSAGE_WIDTH, message);
+
+	INSTR_TIME_SET_CURRENT(step_start);
 }
 
 static void
@@ -280,11 +289,55 @@ pg_fatal(const char *fmt,...)
 }
 
 
+static char *
+get_time_str(double time_ms)
+{
+	double		seconds;
+	double		minutes;
+	double		hours;
+	double		days;
+
+	if (time_ms < 1000.0)
+		return psprintf(_("%.3f ms"), time_ms);
+
+	seconds = time_ms / 1000.0;
+	minutes = floor(seconds / 60.0);
+	seconds -= 60.0 * minutes;
+	if (minutes < 60.0)
+		return psprintf(_("%.3f ms (%02d:%06.3f)"),
+						time_ms, (int) minutes, seconds);
+
+	hours = floor(minutes / 60.0);
+	minutes -= 60.0 * hours;
+	if (hours < 24.0)
+		return psprintf(_("%.3f ms (%02d:%02d:%06.3f)"),
+						time_ms, (int) hours, (int) minutes, seconds);
+
+	days = floor(hours / 24.0);
+	hours -= 24.0 * days;
+	return psprintf(_("%.3f ms (%.0f d %02d:%02d:%06.3f)"),
+					time_ms, days, (int) hours, (int) minutes, seconds);
+}
+
+
 void
 check_ok(void)
 {
+	instr_time	step_end;
+	char	   *step_time;
+
+	Assert(!INSTR_TIME_IS_ZERO(step_start));
+
+	INSTR_TIME_SET_CURRENT(step_end);
+	INSTR_TIME_SUBTRACT(step_end, step_start);
+	step_time = get_time_str(INSTR_TIME_GET_MILLISEC(step_end));
+
+	/* reset start time */
+	INSTR_TIME_SET_ZERO(step_start);
+
 	/* all seems well */
-	report_status(PG_REPORT, "ok");
+	report_status(PG_REPORT, "ok\t%s", step_time);
+	pfree(step_time);
 }
 
 
-- 
2.25.1


--AhhlLboLdkugWU4S--





^ permalink  raw  reply  [nested|flat] 21+ messages in thread

* [PATCH v1 1/1] add timing information to pg_upgrade
@ 2023-07-27 23:16  Nathan Bossart <[email protected]>
  0 siblings, 0 replies; 21+ messages in thread

From: Nathan Bossart @ 2023-07-27 23:16 UTC (permalink / raw)

---
 src/bin/pg_upgrade/util.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/src/bin/pg_upgrade/util.c b/src/bin/pg_upgrade/util.c
index 21ba4c8f12..d1b506a741 100644
--- a/src/bin/pg_upgrade/util.c
+++ b/src/bin/pg_upgrade/util.c
@@ -16,6 +16,8 @@
 
 LogOpts		log_opts;
 
+static struct timeval step_start;
+
 static void pg_log_v(eLogType type, const char *fmt, va_list ap) pg_attribute_printf(2, 0);
 
 
@@ -137,6 +139,8 @@ prep_status(const char *fmt,...)
 
 	/* trim strings */
 	pg_log(PG_REPORT_NONL, "%-*s", MESSAGE_WIDTH, message);
+
+	gettimeofday(&step_start, NULL);
 }
 
 /*
@@ -170,6 +174,8 @@ prep_status_progress(const char *fmt,...)
 		pg_log(PG_REPORT, "%-*s", MESSAGE_WIDTH, message);
 	else
 		pg_log(PG_REPORT_NONL, "%-*s", MESSAGE_WIDTH, message);
+
+	gettimeofday(&step_start, NULL);
 }
 
 static void
@@ -283,8 +289,19 @@ pg_fatal(const char *fmt,...)
 void
 check_ok(void)
 {
+	struct timeval step_end;
+	int64		elapsed_ms;
+	int64		start_ms;
+	int64		end_ms;
+
+	gettimeofday(&step_end, NULL);
+
+	start_ms = (step_start.tv_sec * 1000L) + (step_start.tv_usec / 1000L);
+	end_ms = (step_end.tv_sec * 1000L) + (step_end.tv_usec / 1000L);
+	elapsed_ms = end_ms - start_ms;
+
 	/* all seems well */
-	report_status(PG_REPORT, "ok");
+	report_status(PG_REPORT, "ok (took %ld ms)", elapsed_ms);
 }
 
 
-- 
2.25.1


--gBBFr7Ir9EOA20Yy--





^ permalink  raw  reply  [nested|flat] 21+ messages in thread

* [PATCH v3 2/2] add timing information to pg_upgrade
@ 2023-07-27 23:16  Nathan Bossart <[email protected]>
  0 siblings, 0 replies; 21+ messages in thread

From: Nathan Bossart @ 2023-07-27 23:16 UTC (permalink / raw)

---
 src/bin/pg_upgrade/util.c | 55 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 54 insertions(+), 1 deletion(-)

diff --git a/src/bin/pg_upgrade/util.c b/src/bin/pg_upgrade/util.c
index 21ba4c8f12..23fd5f87af 100644
--- a/src/bin/pg_upgrade/util.c
+++ b/src/bin/pg_upgrade/util.c
@@ -9,14 +9,19 @@
 
 #include "postgres_fe.h"
 
+#include <math.h>
 #include <signal.h>
 
 #include "common/username.h"
 #include "pg_upgrade.h"
+#include "portability/instr_time.h"
 
 LogOpts		log_opts;
 
+static instr_time step_start;
+
 static void pg_log_v(eLogType type, const char *fmt, va_list ap) pg_attribute_printf(2, 0);
+static char *get_time_str(double time_ms);
 
 
 /*
@@ -137,6 +142,8 @@ prep_status(const char *fmt,...)
 
 	/* trim strings */
 	pg_log(PG_REPORT_NONL, "%-*s", MESSAGE_WIDTH, message);
+
+	INSTR_TIME_SET_CURRENT(step_start);
 }
 
 /*
@@ -170,6 +177,8 @@ prep_status_progress(const char *fmt,...)
 		pg_log(PG_REPORT, "%-*s", MESSAGE_WIDTH, message);
 	else
 		pg_log(PG_REPORT_NONL, "%-*s", MESSAGE_WIDTH, message);
+
+	INSTR_TIME_SET_CURRENT(step_start);
 }
 
 static void
@@ -280,11 +289,55 @@ pg_fatal(const char *fmt,...)
 }
 
 
+static char *
+get_time_str(double time_ms)
+{
+	double		seconds;
+	double		minutes;
+	double		hours;
+	double		days;
+
+	if (time_ms < 1000.0)
+		return psprintf(_("%.3f ms"), time_ms);
+
+	seconds = time_ms / 1000.0;
+	minutes = floor(seconds / 60.0);
+	seconds -= 60.0 * minutes;
+	if (minutes < 60.0)
+		return psprintf(_("%.3f ms (%02d:%06.3f)"),
+						time_ms, (int) minutes, seconds);
+
+	hours = floor(minutes / 60.0);
+	minutes -= 60.0 * hours;
+	if (hours < 24.0)
+		return psprintf(_("%.3f ms (%02d:%02d:%06.3f)"),
+						time_ms, (int) hours, (int) minutes, seconds);
+
+	days = floor(hours / 24.0);
+	hours -= 24.0 * days;
+	return psprintf(_("%.3f ms (%.0f d %02d:%02d:%06.3f)"),
+					time_ms, days, (int) hours, (int) minutes, seconds);
+}
+
+
 void
 check_ok(void)
 {
+	instr_time	step_end;
+	char	   *step_time;
+
+	Assert(!INSTR_TIME_IS_ZERO(step_start));
+
+	INSTR_TIME_SET_CURRENT(step_end);
+	INSTR_TIME_SUBTRACT(step_end, step_start);
+	step_time = get_time_str(INSTR_TIME_GET_MILLISEC(step_end));
+
+	/* reset start time */
+	INSTR_TIME_SET_ZERO(step_start);
+
 	/* all seems well */
-	report_status(PG_REPORT, "ok");
+	report_status(PG_REPORT, "ok\t%s", step_time);
+	pfree(step_time);
 }
 
 
-- 
2.25.1


--AhhlLboLdkugWU4S--





^ permalink  raw  reply  [nested|flat] 21+ messages in thread

* [PATCH v2 1/1] add timing information to pg_upgrade
@ 2023-07-27 23:16  Nathan Bossart <[email protected]>
  0 siblings, 0 replies; 21+ messages in thread

From: Nathan Bossart @ 2023-07-27 23:16 UTC (permalink / raw)

---
 src/bin/pg_upgrade/util.c | 55 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 54 insertions(+), 1 deletion(-)

diff --git a/src/bin/pg_upgrade/util.c b/src/bin/pg_upgrade/util.c
index 21ba4c8f12..25e43a593a 100644
--- a/src/bin/pg_upgrade/util.c
+++ b/src/bin/pg_upgrade/util.c
@@ -9,14 +9,19 @@
 
 #include "postgres_fe.h"
 
+#include <math.h>
 #include <signal.h>
 
 #include "common/username.h"
 #include "pg_upgrade.h"
+#include "portability/instr_time.h"
 
 LogOpts		log_opts;
 
+static instr_time step_start;
+
 static void pg_log_v(eLogType type, const char *fmt, va_list ap) pg_attribute_printf(2, 0);
+static char *get_time_str(double time_ms);
 
 
 /*
@@ -137,6 +142,8 @@ prep_status(const char *fmt,...)
 
 	/* trim strings */
 	pg_log(PG_REPORT_NONL, "%-*s", MESSAGE_WIDTH, message);
+
+	INSTR_TIME_SET_CURRENT(step_start);
 }
 
 /*
@@ -170,6 +177,8 @@ prep_status_progress(const char *fmt,...)
 		pg_log(PG_REPORT, "%-*s", MESSAGE_WIDTH, message);
 	else
 		pg_log(PG_REPORT_NONL, "%-*s", MESSAGE_WIDTH, message);
+
+	INSTR_TIME_SET_CURRENT(step_start);
 }
 
 static void
@@ -280,11 +289,55 @@ pg_fatal(const char *fmt,...)
 }
 
 
+static char *
+get_time_str(double time_ms)
+{
+	double		seconds;
+	double		minutes;
+	double		hours;
+	double		days;
+
+	if (time_ms < 1000.0)
+		return psprintf("%.3f ms", time_ms);
+
+	seconds = time_ms / 1000.0;
+	minutes = floor(seconds / 60.0);
+	seconds -= 60.0 * minutes;
+	if (minutes < 60.0)
+		return psprintf("%.3f ms (%02d:%06.3f)",
+						time_ms, (int) minutes, seconds);
+
+	hours = floor(minutes / 60.0);
+	minutes -= 60.0 * hours;
+	if (hours < 24.0)
+		return psprintf("%.3f ms (%02d:%02d:%06.3f)",
+						time_ms, (int) hours, (int) minutes, seconds);
+
+	days = floor(hours / 24.0);
+	hours -= 24.0 * days;
+	return psprintf("%.3f ms (%.0f d %02d:%02d:%06.3f)",
+					time_ms, days, (int) hours, (int) minutes, seconds);
+}
+
+
 void
 check_ok(void)
 {
+	instr_time	step_end;
+	char	   *step_time;
+
+	Assert(!INSTR_TIME_IS_ZERO(step_start));
+
+	INSTR_TIME_SET_CURRENT(step_end);
+	INSTR_TIME_SUBTRACT(step_end, step_start);
+	step_time = get_time_str(INSTR_TIME_GET_MILLISEC(step_end));
+
+	/* reset start time */
+	INSTR_TIME_SET_ZERO(step_start);
+
 	/* all seems well */
-	report_status(PG_REPORT, "ok");
+	report_status(PG_REPORT, "ok\t%s", step_time);
+	pfree(step_time);
 }
 
 
-- 
2.25.1


--vtzGhvizbBRQ85DL--





^ permalink  raw  reply  [nested|flat] 21+ messages in thread

* [PATCH v1 1/1] add timing information to pg_upgrade
@ 2023-07-27 23:16  Nathan Bossart <[email protected]>
  0 siblings, 0 replies; 21+ messages in thread

From: Nathan Bossart @ 2023-07-27 23:16 UTC (permalink / raw)

---
 src/bin/pg_upgrade/util.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/src/bin/pg_upgrade/util.c b/src/bin/pg_upgrade/util.c
index 21ba4c8f12..d1b506a741 100644
--- a/src/bin/pg_upgrade/util.c
+++ b/src/bin/pg_upgrade/util.c
@@ -16,6 +16,8 @@
 
 LogOpts		log_opts;
 
+static struct timeval step_start;
+
 static void pg_log_v(eLogType type, const char *fmt, va_list ap) pg_attribute_printf(2, 0);
 
 
@@ -137,6 +139,8 @@ prep_status(const char *fmt,...)
 
 	/* trim strings */
 	pg_log(PG_REPORT_NONL, "%-*s", MESSAGE_WIDTH, message);
+
+	gettimeofday(&step_start, NULL);
 }
 
 /*
@@ -170,6 +174,8 @@ prep_status_progress(const char *fmt,...)
 		pg_log(PG_REPORT, "%-*s", MESSAGE_WIDTH, message);
 	else
 		pg_log(PG_REPORT_NONL, "%-*s", MESSAGE_WIDTH, message);
+
+	gettimeofday(&step_start, NULL);
 }
 
 static void
@@ -283,8 +289,19 @@ pg_fatal(const char *fmt,...)
 void
 check_ok(void)
 {
+	struct timeval step_end;
+	int64		elapsed_ms;
+	int64		start_ms;
+	int64		end_ms;
+
+	gettimeofday(&step_end, NULL);
+
+	start_ms = (step_start.tv_sec * 1000L) + (step_start.tv_usec / 1000L);
+	end_ms = (step_end.tv_sec * 1000L) + (step_end.tv_usec / 1000L);
+	elapsed_ms = end_ms - start_ms;
+
 	/* all seems well */
-	report_status(PG_REPORT, "ok");
+	report_status(PG_REPORT, "ok (took %ld ms)", elapsed_ms);
 }
 
 
-- 
2.25.1


--gBBFr7Ir9EOA20Yy--





^ permalink  raw  reply  [nested|flat] 21+ messages in thread

* Re: Patch bug: Fix jsonpath .* on Arrays
@ 2024-06-25 17:48  David E. Wheeler <[email protected]>
  0 siblings, 1 reply; 21+ messages in thread

From: David E. Wheeler @ 2024-06-25 17:48 UTC (permalink / raw)
  To: Степан Неретин <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>; David G. Johnston <[email protected]>

On Jun 25, 2024, at 12:46 AM, Степан Неретин <[email protected]> wrote:

> Hi! Looks good to me, but I have several comments.

Thanks for your review!

> Your patch improves tests, but why did you change formatting in jsonpath_exec.c? What's the motivation?

It’s not just formatting. From the commit message:

> While at it, teach `executeAnyItem()` to return `jperOk` when `found`
> exist, not because it will be used (the result and `found` are inspected
> by different functions), but because it seems like the proper thing to
> return from `executeAnyItem()` when considered in isolation.


I have since realized it’s not a complete fix for the issue, and hacked around it in my Go version. Would be fine to remove that bit, but IIRC this was the only execution function that would return `jperNotFound` when it in fact adds items to the `found` list. The current implementation only looks at one or the other, so it’s not super important, but I found the inconsistency annoying and sometimes confusing.

>  [1] select jsonb_path_query('[1,2,3,{"b": [3,4,5]}]', 'strict $.*');
> I propose adding a similar test with explicitly specified lax mode: select jsonb_path_query('[1,2,3,{"b": [3,4,5]}]', 'lax $.*'); to show what lax mode is set by default.

Very few of the other tests do so; I can add it if it’s important for this case, though.

> Odd empty result for the test: select jsonb '[1,2,3,{"b": [3,4,5]}]' @? 'strict $.*';
> I expected an error like in test [1]. This behavior is not obvious to me.

@? suppresses a number of errors. Perhaps I should add a variant of the error-raising query that passes the silent arg, which would also suppress the error.

> Everything else is cool. Thanks to the patch and the discussion above, I began to understand better how wildcards in JSON work.

Yeah, it’s kind of wild TBH.

Best,

David







^ permalink  raw  reply  [nested|flat] 21+ messages in thread

* Re: Patch bug: Fix jsonpath .* on Arrays
@ 2024-06-26 18:16  David E. Wheeler <[email protected]>
  parent: David E. Wheeler <[email protected]>
  0 siblings, 1 reply; 21+ messages in thread

From: David E. Wheeler @ 2024-06-26 18:16 UTC (permalink / raw)
  To: Степан Неретин <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>; David G. Johnston <[email protected]>

On Jun 25, 2024, at 13:48, David E. Wheeler <[email protected]> wrote:

> I have since realized it’s not a complete fix for the issue, and hacked around it in my Go version. Would be fine to remove that bit, but IIRC this was the only execution function that would return `jperNotFound` when it in fact adds items to the `found` list. The current implementation only looks at one or the other, so it’s not super important, but I found the inconsistency annoying and sometimes confusing.

I’ve removed this change.

>> [1] select jsonb_path_query('[1,2,3,{"b": [3,4,5]}]', 'strict $.*');
>> I propose adding a similar test with explicitly specified lax mode: select jsonb_path_query('[1,2,3,{"b": [3,4,5]}]', 'lax $.*'); to show what lax mode is set by default.
> 
> Very few of the other tests do so; I can add it if it’s important for this case, though.

Went ahead and added lax.

> @? suppresses a number of errors. Perhaps I should add a variant of the error-raising query that passes the silent arg, which would also suppress the error.

Added a variant where the silent param suppresses the error, too.

V2 attached and the PR updated:

  https://github.com/theory/postgres/pull/4/files

Best,

David






Attachments:

  [application/octet-stream] v2-0001-Add-tests-for-jsonpath-.-on-arrays.patch (4.1K, ../../[email protected]/2-v2-0001-Add-tests-for-jsonpath-.-on-arrays.patch)
  download | inline diff:
From a7f9c1e4261d4279a4b38ba8ef903cc0e0707ca5 Mon Sep 17 00:00:00 2001
From: "David E. Wheeler" <[email protected]>
Date: Wed, 26 Jun 2024 14:12:47 -0400
Subject: [PATCH v2] Add tests for jsonpath `.*` on arrays

There was no coverage for the path to unwrap an array before applying
`.*` to it, so add tests that explicitly test `.*` for both objects and
arrays, showing how no results are returned for an array of scalars, but
results are returned when the array contains an object. Also test the
behavior in strict mode with and without the silent parameter, and with
the `@?` operator.

Unrelated but potentially useful to future source readers: document
`GetJsonPathVar` and `CountJsonPathVars`.
---
 src/backend/utils/adt/jsonpath_exec.c        |  7 ++-
 src/test/regress/expected/jsonb_jsonpath.out | 50 ++++++++++++++++++++
 src/test/regress/sql/jsonb_jsonpath.sql      | 11 +++++
 3 files changed, 67 insertions(+), 1 deletion(-)

diff --git a/src/backend/utils/adt/jsonpath_exec.c b/src/backend/utils/adt/jsonpath_exec.c
index d79c929822..bf69f0d824 100644
--- a/src/backend/utils/adt/jsonpath_exec.c
+++ b/src/backend/utils/adt/jsonpath_exec.c
@@ -2978,7 +2978,8 @@ getJsonPathItem(JsonPathExecContext *cxt, JsonPathItem *item,
 }
 
 /*
- * Returns the computed value of a JSON path variable with given name.
+ * Definition of JsonPathGetVarCallback for when JsonPathExecContext.vars
+ * is specified as a List value.
  */
 static JsonbValue *
 GetJsonPathVar(void *cxt, char *varName, int varNameLen,
@@ -3025,6 +3026,10 @@ GetJsonPathVar(void *cxt, char *varName, int varNameLen,
 	return result;
 }
 
+/*
+ * Definition of JsonPathCountVarsCallback for when JsonPathExecContext.vars
+ * is specified as a List value.
+ */
 static int
 CountJsonPathVars(void *cxt)
 {
diff --git a/src/test/regress/expected/jsonb_jsonpath.out b/src/test/regress/expected/jsonb_jsonpath.out
index a6112e86fa..3dea937887 100644
--- a/src/test/regress/expected/jsonb_jsonpath.out
+++ b/src/test/regress/expected/jsonb_jsonpath.out
@@ -4394,3 +4394,53 @@ ORDER BY s1.num, s2.num;
  {"s": "B"}    | {"s": "B"}    | false | true  | true  | true  | false
 (144 rows)
 
+-- Test any key on arrays with and without unwrapping.
+select jsonb_path_query('{"a": [1,2,3], "b": [3,4,5]}', '$.*');
+ jsonb_path_query 
+------------------
+ [1, 2, 3]
+ [3, 4, 5]
+(2 rows)
+
+select jsonb_path_query('[1,2,3]', '$.*');
+ jsonb_path_query 
+------------------
+(0 rows)
+
+select jsonb_path_query('[1,2,3,{"b": [3,4,5]}]', 'lax $.*');
+ jsonb_path_query 
+------------------
+ [3, 4, 5]
+(1 row)
+
+select jsonb_path_query('[1,2,3,{"b": [3,4,5]}]', 'strict $.*');
+ERROR:  jsonpath wildcard member accessor can only be applied to an object
+select jsonb_path_query('[1,2,3,{"b": [3,4,5]}]', 'strict $.*', NULL, true);
+ jsonb_path_query 
+------------------
+(0 rows)
+
+select jsonb '{"a": [1,2,3], "b": [3,4,5]}' @? '$.*';
+ ?column? 
+----------
+ t
+(1 row)
+
+select jsonb '[1,2,3]' @? '$.*';
+ ?column? 
+----------
+ f
+(1 row)
+
+select jsonb '[1,2,3,{"b": [3,4,5]}]' @? 'lax $.*';
+ ?column? 
+----------
+ t
+(1 row)
+
+select jsonb '[1,2,3,{"b": [3,4,5]}]' @? 'strict $.*';
+ ?column? 
+----------
+ 
+(1 row)
+
diff --git a/src/test/regress/sql/jsonb_jsonpath.sql b/src/test/regress/sql/jsonb_jsonpath.sql
index 5e14f7759b..f60bccc654 100644
--- a/src/test/regress/sql/jsonb_jsonpath.sql
+++ b/src/test/regress/sql/jsonb_jsonpath.sql
@@ -1116,3 +1116,14 @@ SELECT
 	jsonb_path_query_first(s1.j, '$.s > $s', vars => s2.j) gt
 FROM str s1, str s2
 ORDER BY s1.num, s2.num;
+
+-- Test any key on arrays with and without unwrapping.
+select jsonb_path_query('{"a": [1,2,3], "b": [3,4,5]}', '$.*');
+select jsonb_path_query('[1,2,3]', '$.*');
+select jsonb_path_query('[1,2,3,{"b": [3,4,5]}]', 'lax $.*');
+select jsonb_path_query('[1,2,3,{"b": [3,4,5]}]', 'strict $.*');
+select jsonb_path_query('[1,2,3,{"b": [3,4,5]}]', 'strict $.*', NULL, true);
+select jsonb '{"a": [1,2,3], "b": [3,4,5]}' @? '$.*';
+select jsonb '[1,2,3]' @? '$.*';
+select jsonb '[1,2,3,{"b": [3,4,5]}]' @? 'lax $.*';
+select jsonb '[1,2,3,{"b": [3,4,5]}]' @? 'strict $.*';
-- 
2.45.2



^ permalink  raw  reply  [nested|flat] 21+ messages in thread

* Re: Patch bug: Fix jsonpath .* on Arrays
@ 2024-06-27 04:53  Stepan Neretin <[email protected]>
  parent: David E. Wheeler <[email protected]>
  0 siblings, 1 reply; 21+ messages in thread

From: Stepan Neretin @ 2024-06-27 04:53 UTC (permalink / raw)
  To: David E. Wheeler <[email protected]>; +Cc: Степан Неретин <[email protected]>; PostgreSQL Hackers <[email protected]>; David G. Johnston <[email protected]>

On Thu, Jun 27, 2024 at 1:16 AM David E. Wheeler <[email protected]>
wrote:

> On Jun 25, 2024, at 13:48, David E. Wheeler <[email protected]> wrote:
>
> > I have since realized it’s not a complete fix for the issue, and hacked
> around it in my Go version. Would be fine to remove that bit, but IIRC this
> was the only execution function that would return `jperNotFound` when it in
> fact adds items to the `found` list. The current implementation only looks
> at one or the other, so it’s not super important, but I found the
> inconsistency annoying and sometimes confusing.
>
> I’ve removed this change.
>
> >> [1] select jsonb_path_query('[1,2,3,{"b": [3,4,5]}]', 'strict $.*');
> >> I propose adding a similar test with explicitly specified lax mode:
> select jsonb_path_query('[1,2,3,{"b": [3,4,5]}]', 'lax $.*'); to show what
> lax mode is set by default.
> >
> > Very few of the other tests do so; I can add it if it’s important for
> this case, though.
>
> Went ahead and added lax.
>
> > @? suppresses a number of errors. Perhaps I should add a variant of the
> error-raising query that passes the silent arg, which would also suppress
> the error.
>
> Added a variant where the silent param suppresses the error, too.
>
> V2 attached and the PR updated:
>
>   https://github.com/theory/postgres/pull/4/files
>
> Best,
>
> David
>
>
>
>
HI! Now it looks good for me.
Best regards, Stepan Neretin.


^ permalink  raw  reply  [nested|flat] 21+ messages in thread

* Re: Patch bug: Fix jsonpath .* on Arrays
@ 2024-06-27 08:17  Michael Paquier <[email protected]>
  parent: Stepan Neretin <[email protected]>
  0 siblings, 2 replies; 21+ messages in thread

From: Michael Paquier @ 2024-06-27 08:17 UTC (permalink / raw)
  To: Stepan Neretin <[email protected]>; +Cc: David E. Wheeler <[email protected]>; Степан Неретин <[email protected]>; PostgreSQL Hackers <[email protected]>; David G. Johnston <[email protected]>

On Thu, Jun 27, 2024 at 11:53:14AM +0700, Stepan Neretin wrote:
> HI! Now it looks good for me.

The tests of jsonb_jsonpath.sql include a lot of patterns for @? and
jsonb_path_query with the lax and strict modes, so shouldn't these
additions be grouped closer to the existing tests rather than added at 
the end of the file?
--
Michael


Attachments:

  [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
  download

^ permalink  raw  reply  [nested|flat] 21+ messages in thread

* Re: Patch bug: Fix jsonpath .* on Arrays
@ 2024-06-27 15:05  David E. Wheeler <[email protected]>
  parent: Michael Paquier <[email protected]>
  1 sibling, 0 replies; 21+ messages in thread

From: David E. Wheeler @ 2024-06-27 15:05 UTC (permalink / raw)
  To: Michael Paquier <[email protected]>; +Cc: Stepan Neretin <[email protected]>; Степан Неретин <[email protected]>; PostgreSQL Hackers <[email protected]>; David G. Johnston <[email protected]>

On Jun 27, 2024, at 04:17, Michael Paquier <[email protected]> wrote:

> The tests of jsonb_jsonpath.sql include a lot of patterns for @? and
> jsonb_path_query with the lax and strict modes, so shouldn't these
> additions be grouped closer to the existing tests rather than added at 
> the end of the file?

I think you could argue that they should go with other tests for array unwrapping, though it’s kind of mixed throughout. But that’s more the bit I was testing; almost all the tests are lax, and it’s less the strict behavior to test here than the explicit behavior of unwrapping in lax mode.

But ultimately I don’t care where they go, just that we have them.

D







^ permalink  raw  reply  [nested|flat] 21+ messages in thread

* Re: Patch bug: Fix jsonpath .* on Arrays
@ 2024-07-08 16:09  David E. Wheeler <[email protected]>
  parent: Michael Paquier <[email protected]>
  1 sibling, 1 reply; 21+ messages in thread

From: David E. Wheeler @ 2024-07-08 16:09 UTC (permalink / raw)
  To: Michael Paquier <[email protected]>; +Cc: Stepan Neretin <[email protected]>; Степан Неретин <[email protected]>; PostgreSQL Hackers <[email protected]>; David G. Johnston <[email protected]>

On Jun 27, 2024, at 04:17, Michael Paquier <[email protected]> wrote:

> The tests of jsonb_jsonpath.sql include a lot of patterns for @? and
> jsonb_path_query with the lax and strict modes, so shouldn't these
> additions be grouped closer to the existing tests rather than added at 
> the end of the file?

I’ve moved them closer to other tests for unwrapping behavior in the attached updated and rebased v3 patch.

Best,

David






Attachments:

  [application/octet-stream] v3-0001-Add-tests-for-jsonpath-.-on-arrays.patch (4.3K, ../../[email protected]/2-v3-0001-Add-tests-for-jsonpath-.-on-arrays.patch)
  download | inline diff:
From eefe7a08e7c365082954c0026a13dad41f713744 Mon Sep 17 00:00:00 2001
From: "David E. Wheeler" <[email protected]>
Date: Mon, 8 Jul 2024 12:08:00 -0400
Subject: [PATCH v3] Add tests for jsonpath `.*` on arrays

There was no coverage for the path to unwrap an array before applying
`.*` to it, so add tests that explicitly test `.*` for both objects and
arrays, showing how no results are returned for an array of scalars, but
results are returned when the array contains an object. Also test the
behavior in strict mode with and without the silent parameter, and with
the `@?` operator.

Unrelated but potentially useful to future source readers: document
`GetJsonPathVar` and `CountJsonPathVars`.
---
 src/backend/utils/adt/jsonpath_exec.c        |  7 ++-
 src/test/regress/expected/jsonb_jsonpath.out | 50 ++++++++++++++++++++
 src/test/regress/sql/jsonb_jsonpath.sql      | 11 +++++
 3 files changed, 67 insertions(+), 1 deletion(-)

diff --git a/src/backend/utils/adt/jsonpath_exec.c b/src/backend/utils/adt/jsonpath_exec.c
index d79c929822..bf69f0d824 100644
--- a/src/backend/utils/adt/jsonpath_exec.c
+++ b/src/backend/utils/adt/jsonpath_exec.c
@@ -2978,7 +2978,8 @@ getJsonPathItem(JsonPathExecContext *cxt, JsonPathItem *item,
 }
 
 /*
- * Returns the computed value of a JSON path variable with given name.
+ * Definition of JsonPathGetVarCallback for when JsonPathExecContext.vars
+ * is specified as a List value.
  */
 static JsonbValue *
 GetJsonPathVar(void *cxt, char *varName, int varNameLen,
@@ -3025,6 +3026,10 @@ GetJsonPathVar(void *cxt, char *varName, int varNameLen,
 	return result;
 }
 
+/*
+ * Definition of JsonPathCountVarsCallback for when JsonPathExecContext.vars
+ * is specified as a List value.
+ */
 static int
 CountJsonPathVars(void *cxt)
 {
diff --git a/src/test/regress/expected/jsonb_jsonpath.out b/src/test/regress/expected/jsonb_jsonpath.out
index a6112e86fa..7bb4eb1bc2 100644
--- a/src/test/regress/expected/jsonb_jsonpath.out
+++ b/src/test/regress/expected/jsonb_jsonpath.out
@@ -1135,6 +1135,56 @@ select jsonb_path_query('{"a": [1, 2]}', 'lax $.a * 3', silent => true);
 ------------------
 (0 rows)
 
+-- any key on arrays with and without unwrapping.
+select jsonb_path_query('{"a": [1,2,3], "b": [3,4,5]}', '$.*');
+ jsonb_path_query 
+------------------
+ [1, 2, 3]
+ [3, 4, 5]
+(2 rows)
+
+select jsonb_path_query('[1,2,3]', '$.*');
+ jsonb_path_query 
+------------------
+(0 rows)
+
+select jsonb_path_query('[1,2,3,{"b": [3,4,5]}]', 'lax $.*');
+ jsonb_path_query 
+------------------
+ [3, 4, 5]
+(1 row)
+
+select jsonb_path_query('[1,2,3,{"b": [3,4,5]}]', 'strict $.*');
+ERROR:  jsonpath wildcard member accessor can only be applied to an object
+select jsonb_path_query('[1,2,3,{"b": [3,4,5]}]', 'strict $.*', NULL, true);
+ jsonb_path_query 
+------------------
+(0 rows)
+
+select jsonb '{"a": [1,2,3], "b": [3,4,5]}' @? '$.*';
+ ?column? 
+----------
+ t
+(1 row)
+
+select jsonb '[1,2,3]' @? '$.*';
+ ?column? 
+----------
+ f
+(1 row)
+
+select jsonb '[1,2,3,{"b": [3,4,5]}]' @? 'lax $.*';
+ ?column? 
+----------
+ t
+(1 row)
+
+select jsonb '[1,2,3,{"b": [3,4,5]}]' @? 'strict $.*';
+ ?column? 
+----------
+ 
+(1 row)
+
 -- extension: boolean expressions
 select jsonb_path_query('2', '$ > 1');
  jsonb_path_query 
diff --git a/src/test/regress/sql/jsonb_jsonpath.sql b/src/test/regress/sql/jsonb_jsonpath.sql
index 5e14f7759b..17f9d038c0 100644
--- a/src/test/regress/sql/jsonb_jsonpath.sql
+++ b/src/test/regress/sql/jsonb_jsonpath.sql
@@ -241,6 +241,17 @@ select jsonb_path_query('{"a": [2, 3, 4]}', 'lax -$.a');
 select jsonb_path_query('{"a": [1, 2]}', 'lax $.a * 3');
 select jsonb_path_query('{"a": [1, 2]}', 'lax $.a * 3', silent => true);
 
+-- any key on arrays with and without unwrapping.
+select jsonb_path_query('{"a": [1,2,3], "b": [3,4,5]}', '$.*');
+select jsonb_path_query('[1,2,3]', '$.*');
+select jsonb_path_query('[1,2,3,{"b": [3,4,5]}]', 'lax $.*');
+select jsonb_path_query('[1,2,3,{"b": [3,4,5]}]', 'strict $.*');
+select jsonb_path_query('[1,2,3,{"b": [3,4,5]}]', 'strict $.*', NULL, true);
+select jsonb '{"a": [1,2,3], "b": [3,4,5]}' @? '$.*';
+select jsonb '[1,2,3]' @? '$.*';
+select jsonb '[1,2,3,{"b": [3,4,5]}]' @? 'lax $.*';
+select jsonb '[1,2,3,{"b": [3,4,5]}]' @? 'strict $.*';
+
 -- extension: boolean expressions
 select jsonb_path_query('2', '$ > 1');
 select jsonb_path_query('2', '$ <= 1');
-- 
2.45.2



^ permalink  raw  reply  [nested|flat] 21+ messages in thread

* Re: Patch bug: Fix jsonpath .* on Arrays
@ 2024-07-15 11:07  Stepan Neretin <[email protected]>
  parent: David E. Wheeler <[email protected]>
  0 siblings, 1 reply; 21+ messages in thread

From: Stepan Neretin @ 2024-07-15 11:07 UTC (permalink / raw)
  To: David E. Wheeler <[email protected]>; +Cc: Michael Paquier <[email protected]>; Степан Неретин <[email protected]>; PostgreSQL Hackers <[email protected]>; David G. Johnston <[email protected]>

On Mon, Jul 8, 2024 at 11:09 PM David E. Wheeler <[email protected]>
wrote:

> On Jun 27, 2024, at 04:17, Michael Paquier <[email protected]> wrote:
>
> > The tests of jsonb_jsonpath.sql include a lot of patterns for @? and
> > jsonb_path_query with the lax and strict modes, so shouldn't these
> > additions be grouped closer to the existing tests rather than added at
> > the end of the file?
>
> I’ve moved them closer to other tests for unwrapping behavior in the
> attached updated and rebased v3 patch.
>
> Best,
>
> David
>
>
>

Hi! Looks good to me now! Please, register a patch in CF.
Best regards, Stepan Neretin.


^ permalink  raw  reply  [nested|flat] 21+ messages in thread

* Re: Patch bug: Fix jsonpath .* on Arrays
@ 2024-07-15 14:29  David E. Wheeler <[email protected]>
  parent: Stepan Neretin <[email protected]>
  0 siblings, 1 reply; 21+ messages in thread

From: David E. Wheeler @ 2024-07-15 14:29 UTC (permalink / raw)
  To: Stepan Neretin <[email protected]>; +Cc: Michael Paquier <[email protected]>; Степан Неретин <[email protected]>; PostgreSQL Hackers <[email protected]>; David G. Johnston <[email protected]>

On Jul 15, 2024, at 07:07, Stepan Neretin <[email protected]> wrote:

> Hi! Looks good to me now! Please, register a patch in CF.
> Best regards, Stepan Neretin.

It’s here:

  https://commitfest.postgresql.org/48/5017/

Best,

David







^ permalink  raw  reply  [nested|flat] 21+ messages in thread

* Re: Patch bug: Fix jsonpath .* on Arrays
@ 2024-07-19 05:42  Michael Paquier <[email protected]>
  parent: David E. Wheeler <[email protected]>
  0 siblings, 1 reply; 21+ messages in thread

From: Michael Paquier @ 2024-07-19 05:42 UTC (permalink / raw)
  To: David E. Wheeler <[email protected]>; +Cc: Stepan Neretin <[email protected]>; Степан Неретин <[email protected]>; PostgreSQL Hackers <[email protected]>; David G. Johnston <[email protected]>

On Mon, Jul 15, 2024 at 10:29:32AM -0400, David E. Wheeler wrote:
> It’s here:
> 
>   https://commitfest.postgresql.org/48/5017/

Sorry for the delay.  Finally came back to it, and applied the tests.
Thanks!

It was fun to see that HEAD was silenced with the first patch of this
thread that tweaked the behavior with arrays.

Regarding the comments, I have left them out for now.  That may be a
good start, but it also feels like we should do a much better job
overall with the area of jsonpath_exec.c.  One thing that may help as
a start is to reorganize the routines of the file and divide them into
sub-categories, or even go through a deeper refactoring to help
readers go through the existing 4.5k lines of code that are in this
single file..
--
Michael


Attachments:

  [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
  download

^ permalink  raw  reply  [nested|flat] 21+ messages in thread

* Re: Patch bug: Fix jsonpath .* on Arrays
@ 2024-07-19 13:49  David E. Wheeler <[email protected]>
  parent: Michael Paquier <[email protected]>
  0 siblings, 1 reply; 21+ messages in thread

From: David E. Wheeler @ 2024-07-19 13:49 UTC (permalink / raw)
  To: Michael Paquier <[email protected]>; +Cc: Stepan Neretin <[email protected]>; Степан Неретин <[email protected]>; PostgreSQL Hackers <[email protected]>; David G. Johnston <[email protected]>

On Jul 19, 2024, at 01:42, Michael Paquier <[email protected]> wrote:

> Sorry for the delay.  Finally came back to it, and applied the tests.
> Thanks!

Awesome, thank you!

> It was fun to see that HEAD was silenced with the first patch of this
> thread that tweaked the behavior with arrays.

Uh, what? Sorry I don’t follow.

> Regarding the comments, I have left them out for now.  That may be a
> good start, but it also feels like we should do a much better job
> overall with the area of jsonpath_exec.c.

I put them in because it took me a bit to track down that they were among the implementors of JsonPathGetVarCallback as I was porting the code.

> One thing that may help as
> a start is to reorganize the routines of the file and divide them into
> sub-categories, or even go through a deeper refactoring to help
> readers go through the existing 4.5k lines of code that are in this
> single file..

After I got all the tests passing in the port to Go, I split it up into 14 implementation and 15 test files (with all of jsonb_jsonpath.(sql.out) in one file). Was much easier to reason about that way.

https://github.com/theory/sqljson/tree/main/path/exec

Best,

David








^ permalink  raw  reply  [nested|flat] 21+ messages in thread

* Re: Patch bug: Fix jsonpath .* on Arrays
@ 2024-07-22 00:54  Michael Paquier <[email protected]>
  parent: David E. Wheeler <[email protected]>
  0 siblings, 1 reply; 21+ messages in thread

From: Michael Paquier @ 2024-07-22 00:54 UTC (permalink / raw)
  To: David E. Wheeler <[email protected]>; +Cc: Stepan Neretin <[email protected]>; Степан Неретин <[email protected]>; PostgreSQL Hackers <[email protected]>; David G. Johnston <[email protected]>

On Fri, Jul 19, 2024 at 09:49:50AM -0400, David E. Wheeler wrote:
>> It was fun to see that HEAD was silenced with the first patch of this
>> thread that tweaked the behavior with arrays.
> 
> Uh, what? Sorry I don’t follow.

What I mean is that the main regression test suite did not complain on
your original patch posted here:
https://www.postgresql.org/message-id/A95346F9-6147-46E0-809E-532A485D71D6%40justatheory.com

But the new tests showed a difference, and that's enough for me to see
value in the new tests.
--
Michael


Attachments:

  [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
  download

^ permalink  raw  reply  [nested|flat] 21+ messages in thread

* Re: Patch bug: Fix jsonpath .* on Arrays
@ 2024-07-22 02:18  David E. Wheeler <[email protected]>
  parent: Michael Paquier <[email protected]>
  0 siblings, 0 replies; 21+ messages in thread

From: David E. Wheeler @ 2024-07-22 02:18 UTC (permalink / raw)
  To: Michael Paquier <[email protected]>; +Cc: Stepan Neretin <[email protected]>; Степан Неретин <[email protected]>; PostgreSQL Hackers <[email protected]>; David G. Johnston <[email protected]>

On Jul 21, 2024, at 20:54, Michael Paquier <[email protected]> wrote:

> What I mean is that the main regression test suite did not complain on
> your original patch posted here:
> https://www.postgresql.org/message-id/A95346F9-6147-46E0-809E-532A485D71D6%40justatheory.com
> 
> But the new tests showed a difference, and that's enough for me to see
> value in the new tests.

Oh, got it, nice!

Thanks,

David







^ permalink  raw  reply  [nested|flat] 21+ messages in thread


end of thread, other threads:[~2024-07-22 02:18 UTC | newest]

Thread overview: 21+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2019-03-29 00:20 [PATCH] Clean up pg_checksums.sgml Justin Pryzby <[email protected]>
2019-03-29 00:20 [PATCH] Clean up pg_checksums.sgml Justin Pryzby <[email protected]>
2019-04-05 00:39 [PATCH v2 12/12] docs: create or reindex progress of ptn tables Justin Pryzby <[email protected]>
2023-07-27 23:16 [PATCH v3 2/2] add timing information to pg_upgrade Nathan Bossart <[email protected]>
2023-07-27 23:16 [PATCH v3 2/2] add timing information to pg_upgrade Nathan Bossart <[email protected]>
2023-07-27 23:16 [PATCH v2 1/1] add timing information to pg_upgrade Nathan Bossart <[email protected]>
2023-07-27 23:16 [PATCH v2 1/1] add timing information to pg_upgrade Nathan Bossart <[email protected]>
2023-07-27 23:16 [PATCH v1 1/1] add timing information to pg_upgrade Nathan Bossart <[email protected]>
2023-07-27 23:16 [PATCH v1 1/1] add timing information to pg_upgrade Nathan Bossart <[email protected]>
2024-06-25 17:48 Re: Patch bug: Fix jsonpath .* on Arrays David E. Wheeler <[email protected]>
2024-06-26 18:16 ` Re: Patch bug: Fix jsonpath .* on Arrays David E. Wheeler <[email protected]>
2024-06-27 04:53   ` Re: Patch bug: Fix jsonpath .* on Arrays Stepan Neretin <[email protected]>
2024-06-27 08:17     ` Re: Patch bug: Fix jsonpath .* on Arrays Michael Paquier <[email protected]>
2024-06-27 15:05       ` Re: Patch bug: Fix jsonpath .* on Arrays David E. Wheeler <[email protected]>
2024-07-08 16:09       ` Re: Patch bug: Fix jsonpath .* on Arrays David E. Wheeler <[email protected]>
2024-07-15 11:07         ` Re: Patch bug: Fix jsonpath .* on Arrays Stepan Neretin <[email protected]>
2024-07-15 14:29           ` Re: Patch bug: Fix jsonpath .* on Arrays David E. Wheeler <[email protected]>
2024-07-19 05:42             ` Re: Patch bug: Fix jsonpath .* on Arrays Michael Paquier <[email protected]>
2024-07-19 13:49               ` Re: Patch bug: Fix jsonpath .* on Arrays David E. Wheeler <[email protected]>
2024-07-22 00:54                 ` Re: Patch bug: Fix jsonpath .* on Arrays Michael Paquier <[email protected]>
2024-07-22 02:18                   ` Re: Patch bug: Fix jsonpath .* on Arrays David E. Wheeler <[email protected]>

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox