Re: Avoid unused value (src/fe_utils/print.c)
Hi Michael, 04.06.2023 01:42, Michael Paquier wrote: On Sat, Jun 03, 2023 at 03:00:01PM +0300, Alexander Lakhin wrote: Clang' scan-build detects 58 errors "Dead assignment", including that one. Maybe it would be more sensible to eliminate all errors of this class? Depends on if this makes any code changed a bit easier to understand I guess, so that would be a case-by-case analysis. Saying that, the proposed patch seems right while it makes slightly easier to understand the footer print part. It also aligns the code with print_unaligned_vertical(), but I can't see why need_recordsep = true; is a no-op here (scan-build dislikes only need_recordsep = false;). I suspect that removing that line will change the behaviour in cases when need_recordsep = false after the loop "print cells" and the loop "for (footers)" is executed. Best regards, Alexander
Re: [BUG] pg_dump does not properly deal with BEGIN ATOMIC function
On Sat, Jun 3, 2023 at 2:28 PM Tom Lane wrote: > Kirk Wolak writes: > > On Fri, Jun 2, 2023 at 8:16 AM Tom Lane wrote: > > If I comprehend the suggestion, it will label each line with a warning. > > Which implies I have 6 Warnings. > > Right, I'd forgotten that pg_log_warning() will interpose "warning:". > Attached are two more-carefully-thought-out suggestions. The easy > way is to use pg_log_warning_detail(), which produces output like > > pg_dump: warning: could not resolve dependency loop among these items: > pg_dump: detail: FUNCTION a_f (ID 216 OID 40532) > pg_dump: detail: CONSTRAINT a_pkey (ID 3466 OID 40531) > pg_dump: detail: POST-DATA BOUNDARY (ID 3612) > pg_dump: detail: TABLE DATA a (ID 3610 OID 40525) > pg_dump: detail: PRE-DATA BOUNDARY (ID 3611) > > Alternatively, we could assemble the details by hand, as in the > second patch, producing > > pg_dump: warning: could not resolve dependency loop among these items: > FUNCTION a_f (ID 216 OID 40532) > CONSTRAINT a_pkey (ID 3466 OID 40531) > POST-DATA BOUNDARY (ID 3612) > TABLE DATA a (ID 3610 OID 40525) > PRE-DATA BOUNDARY (ID 3611) > > I'm not really sure which of these I like better. The first one > is a much simpler code change, and there is some value in labeling > the output like that. The second patch's output seems less cluttered, > but it's committing a modularity sin by embedding formatting knowledge > at the caller level. Thoughts? > Honestly the double space in front of the strings with either the Original version, or the "detail:" version is great. While I get the "Less Cluttered" version.. It "detaches" it a bit too much from the lead in, for me. Kirk...
Add test module for Table Access Method
Hi all, During the PGCon Unconference session about Table Access Method one missing item pointed out is that currently we lack documentation and examples of TAM. So in order to improve things a bit in this area I'm proposing to add a test module for Table Access Method similar what we already have for Index Access Method. This code is based on the "blackhole_am" implemented by Michael Paquier: https://github.com/michaelpq/pg_plugins/tree/main/blackhole_am Regards, -- Fabrízio de Royes Mello From 217b84f21ec1cdb0ede271d24b7a5863713db949 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabr=C3=ADzio=20de=20Royes=20Mello?= Date: Sat, 3 Jun 2023 17:23:05 -0400 Subject: [PATCH] Add test module for Table Access Method --- src/test/modules/Makefile | 1 + src/test/modules/dummy_table_am/.gitignore| 3 + src/test/modules/dummy_table_am/Makefile | 20 + src/test/modules/dummy_table_am/README| 12 + .../dummy_table_am/dummy_table_am--1.0.sql| 14 + .../modules/dummy_table_am/dummy_table_am.c | 519 ++ .../dummy_table_am/dummy_table_am.control | 5 + .../expected/dummy_table_am.out | 127 + src/test/modules/dummy_table_am/meson.build | 33 ++ .../dummy_table_am/sql/dummy_table_am.sql | 34 ++ src/test/modules/meson.build | 1 + 11 files changed, 769 insertions(+) create mode 100644 src/test/modules/dummy_table_am/.gitignore create mode 100644 src/test/modules/dummy_table_am/Makefile create mode 100644 src/test/modules/dummy_table_am/README create mode 100644 src/test/modules/dummy_table_am/dummy_table_am--1.0.sql create mode 100644 src/test/modules/dummy_table_am/dummy_table_am.c create mode 100644 src/test/modules/dummy_table_am/dummy_table_am.control create mode 100644 src/test/modules/dummy_table_am/expected/dummy_table_am.out create mode 100644 src/test/modules/dummy_table_am/meson.build create mode 100644 src/test/modules/dummy_table_am/sql/dummy_table_am.sql diff --git a/src/test/modules/Makefile b/src/test/modules/Makefile index 6331c976dc..ce982b0e46 100644 --- a/src/test/modules/Makefile +++ b/src/test/modules/Makefile @@ -10,6 +10,7 @@ SUBDIRS = \ delay_execution \ dummy_index_am \ dummy_seclabel \ + dummy_table_am \ libpq_pipeline \ plsample \ snapshot_too_old \ diff --git a/src/test/modules/dummy_table_am/.gitignore b/src/test/modules/dummy_table_am/.gitignore new file mode 100644 index 00..44d119cfcc --- /dev/null +++ b/src/test/modules/dummy_table_am/.gitignore @@ -0,0 +1,3 @@ +# Generated subdirectories +/log/ +/results/ diff --git a/src/test/modules/dummy_table_am/Makefile b/src/test/modules/dummy_table_am/Makefile new file mode 100644 index 00..9ea4a590c6 --- /dev/null +++ b/src/test/modules/dummy_table_am/Makefile @@ -0,0 +1,20 @@ +# src/test/modules/dummy_table_am/Makefile + +MODULES = dummy_table_am + +EXTENSION = dummy_table_am +DATA = dummy_table_am--1.0.sql +PGFILEDESC = "dummy_table_am - table access method template" + +REGRESS = dummy_table_am + +ifdef USE_PGXS +PG_CONFIG = pg_config +PGXS := $(shell $(PG_CONFIG) --pgxs) +include $(PGXS) +else +subdir = src/test/modules/dummy_table_am +top_builddir = ../../../.. +include $(top_builddir)/src/Makefile.global +include $(top_srcdir)/contrib/contrib-global.mk +endif diff --git a/src/test/modules/dummy_table_am/README b/src/test/modules/dummy_table_am/README new file mode 100644 index 00..61510f02fa --- /dev/null +++ b/src/test/modules/dummy_table_am/README @@ -0,0 +1,12 @@ +Dummy Index AM +== + +Dummy index AM is a module for testing any facility usable by an index +access method, whose code is kept a maximum simple. + +This includes tests for all relation option types: +- boolean +- enum +- integer +- real +- strings (with and without NULL as default) diff --git a/src/test/modules/dummy_table_am/dummy_table_am--1.0.sql b/src/test/modules/dummy_table_am/dummy_table_am--1.0.sql new file mode 100644 index 00..aa0fd82e61 --- /dev/null +++ b/src/test/modules/dummy_table_am/dummy_table_am--1.0.sql @@ -0,0 +1,14 @@ +/* src/test/modules/dummy_table_am/dummy_table_am--1.0.sql */ + +-- complain if script is sourced in psql, rather than via CREATE EXTENSION +\echo Use "CREATE EXTENSION dummy_table_am" to load this file. \quit + +CREATE FUNCTION dummy_table_am_handler(internal) +RETURNS table_am_handler +AS 'MODULE_PATHNAME' +LANGUAGE C; + +-- Access method +CREATE ACCESS METHOD dummy_table_am TYPE TABLE HANDLER dummy_table_am_handler; +COMMENT ON ACCESS METHOD dummy_table_am IS 'dummy table access method'; + diff --git a/src/test/modules/dummy_table_am/dummy_table_am.c b/src/test/modules/dummy_table_am/dummy_table_am.c new file mode 100644 index 00..b299fe9c65 --- /dev/null +++ b/src/test/modules/dummy_table_am/dummy_table_am.c @@ -0,0 +1,519 @@ +/*- + * + * dummy_table_am.c
Re: Avoid unused value (src/fe_utils/print.c)
On Sat, Jun 03, 2023 at 03:00:01PM +0300, Alexander Lakhin wrote: > Clang' scan-build detects 58 errors "Dead assignment", including that one. > Maybe it would be more sensible to eliminate all errors of this class? Depends on if this makes any code changed a bit easier to understand I guess, so that would be a case-by-case analysis. Saying that, the proposed patch seems right while it makes slightly easier to understand the footer print part. -- Michael signature.asc Description: PGP signature
Re: New Table Access Methods for Multi and Single Inserts
Hi,
This patch was referenced in a discussion at pgcon, so I thought I'd give it a
look, even though Bharat said that he won't have time to drive it forward...
On 2021-04-19 10:21:36 +0530, Bharath Rupireddy wrote:
> diff --git a/src/backend/access/heap/heapam_handler.c
> b/src/backend/access/heap/heapam_handler.c
> index bd5faf0c1f..655de8e6b7 100644
> --- a/src/backend/access/heap/heapam_handler.c
> +++ b/src/backend/access/heap/heapam_handler.c
> @@ -2558,6 +2558,11 @@ static const TableAmRoutine heapam_methods = {
> .tuple_insert_speculative = heapam_tuple_insert_speculative,
> .tuple_complete_speculative = heapam_tuple_complete_speculative,
> .multi_insert = heap_multi_insert,
> + .tuple_insert_begin = heap_insert_begin,
> + .tuple_insert_v2 = heap_insert_v2,
> + .multi_insert_v2 = heap_multi_insert_v2,
> + .multi_insert_flush = heap_multi_insert_flush,
> + .tuple_insert_end = heap_insert_end,
> .tuple_delete = heapam_tuple_delete,
> .tuple_update = heapam_tuple_update,
> .tuple_lock = heapam_tuple_lock,
I don't think we should have multiple callback for the insertion APIs in
tableam.h. I think it'd be good to continue supporting the old table_*()
functions, but supporting multiple insert APIs in each AM doesn't make much
sense to me.
> +/*
> + * GetTupleSize - Compute the tuple size given a table slot.
> + *
> + * For heap tuple, buffer tuple and minimal tuple slot types return the
> actual
> + * tuple size that exists. For virtual tuple, the size is calculated as the
> + * slot does not have the tuple size. If the computed size exceeds the given
> + * maxsize for the virtual tuple, this function exits, not investing time in
> + * further unnecessary calculation.
> + *
> + * Important Notes:
> + * 1) Size calculation code for virtual slots is being used from
> + * tts_virtual_materialize(), hence ensure to have the same changes or
> fixes
> + * here and also there.
> + * 2) Currently, GetTupleSize() handles the existing heap, buffer, minimal
> and
> + * virtual slots. Ensure to add related code in case any new slot type is
> + *introduced.
> + */
> +inline Size
> +GetTupleSize(TupleTableSlot *slot, Size maxsize)
> +{
> + Size sz = 0;
> + HeapTuple tuple = NULL;
> +
> + if (TTS_IS_HEAPTUPLE(slot))
> + tuple = ((HeapTupleTableSlot *) slot)->tuple;
> + else if(TTS_IS_BUFFERTUPLE(slot))
> + tuple = ((BufferHeapTupleTableSlot *) slot)->base.tuple;
> + else if(TTS_IS_MINIMALTUPLE(slot))
> + tuple = ((MinimalTupleTableSlot *) slot)->tuple;
> + else if(TTS_IS_VIRTUAL(slot))
I think this embeds too much knowledge of the set of slot types in core
code. I don't see why it's needed either?
> diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h
> index 414b6b4d57..2a1470a7b6 100644
> --- a/src/include/access/tableam.h
> +++ b/src/include/access/tableam.h
> @@ -229,6 +229,32 @@ typedef struct TM_IndexDeleteOp
> TM_IndexStatus *status;
> } TM_IndexDeleteOp;
>
> +/* Holds table insert state. */
> +typedef struct TableInsertState
I suspect we should design it to be usable for updates and deletes in the
future, and thus name it TableModifyState.
> +{
> + Relationrel;
> + /* Bulk insert state if requested, otherwise NULL. */
> + struct BulkInsertStateData *bistate;
> + CommandId cid;
Hm - I'm not sure it's a good idea to force the cid to be the same for all
inserts done via one TableInsertState.
> + int options;
> + /* Below members are only used for multi inserts. */
> + /* Array of buffered slots. */
> + TupleTableSlot **mi_slots;
> + /* Number of slots that are currently buffered. */
> + int32 mi_cur_slots;
> + /*
> + * Access method specific information such as parameters that are needed
> + * for buffering and flushing decisions can go here.
> + */
> + void*mistate;
I think we should instead have a generic TableModifyState, which each AM then
embeds into an AM specific AM state. Forcing two very related structs to be
allocated separately doesn't seem wise in this case.
> @@ -1430,6 +1473,50 @@ table_multi_insert(Relation rel, TupleTableSlot
> **slots, int nslots,
> cid, options,
> bistate);
> }
>
> +static inline TableInsertState*
> +table_insert_begin(Relation rel, CommandId cid, int options,
> +bool alloc_bistate, bool is_multi)
Why have alloc_bistate and options?
> +static inline void
> +table_insert_end(TableInsertState *state)
> +{
> + /* Deallocate bulk insert state here, since it's AM independent. */
> + if (state->bistate)
> + FreeBulkInsertState(state->bistate);
> +
> + state->rel->rd_tableam->tuple_insert_end(state);
> +}
Seems like the order in here should be swapped?
Greetings,
Andres Freund
Re: [PATCH] Slight improvement of worker_spi.c example
On Sat, Jun 03, 2023 at 03:34:30PM +0300, Aleksander Alekseev wrote: > Agree. It is a simple example and I don't think it's going to be > useful to make a complicated one out of it. It does not have to be complicated, but I definitely agree that we'd better spend some efforts in improving it as a whole especially knowing that this is mentioned on the docs as an example that one could rely on. > The order of the calls it currently uses however may be extremely > confusing for newcomers. It creates an impression that this particular > order is extremely important while in fact it's not and it takes time > to figure this out. + * The order of PushActiveSnapshot() and SPI_connect() is not really + * important. FWIW, looking at the patch, I don't think that this is particularly useful. -- Michael signature.asc Description: PGP signature
Re: Implement generalized sub routine find_in_log for tap test
On Mon, May 29, 2023 at 07:49:52AM +0530, vignesh C wrote:
> Thanks for the comment, the attached v3 version patch has the changes
> for the same.
-if (find_in_log(
- $node, $log_offset,
- qr/peer authentication is not supported on this platform/))
+if ($node->log_contains(
+ qr/peer authentication is not supported on this platform/),
+ $log_offset,)
This looks like a typo to me, the log offset is eaten.
Except of that, I am on board with log_contains().
There are two things that bugged me with the refactoring related to
connect_ok and connect_fails:
- check_connect_log_contents() is a name too complicated. While
looking at that I have settled to a simpler log_check(), as we could
perfectly use that for something else than connections as long as we
want to check multiple patterns at once.
- The refactoring of the documentation for the routines of Cluster.pm
became incorrect. For example, the patch does not list anymore
log_like and log_unlike for connect_ok.
With all that in mind I have hacked a few adjustments in a 0003,
though I agree with the separation between 0001 and 0002.
033_replay_tsp_drops and 019_replslot_limit are not new to v16, but
003_peer.pl and 035_standby_logical_decoding.pl, making the number of
places where find_in_log() exists twice as much. So I would be
tempted to refactor these tests in v16. Perhaps anybody from the RMT
could comment? We've usually been quite flexible with the tests even
in beta.
Thoughts?
--
Michael
From 49473ab523adf43e82fe71d7e33be478287ad2c8 Mon Sep 17 00:00:00 2001
From: Vignesh C
Date: Fri, 26 May 2023 20:07:30 +0530
Subject: [PATCH v4 1/3] Remove duplicate find_in_log sub routines from tap
tests.
Remove duplicate find_in_log sub routines from tap tests.
---
src/test/authentication/t/003_peer.pl | 17 ++
src/test/perl/PostgreSQL/Test/Cluster.pm | 15 +
src/test/recovery/t/019_replslot_limit.pl | 33 +--
src/test/recovery/t/033_replay_tsp_drops.pl | 15 ++---
.../t/035_standby_logical_decoding.pl | 26 +++
5 files changed, 32 insertions(+), 74 deletions(-)
diff --git a/src/test/authentication/t/003_peer.pl
b/src/test/authentication/t/003_peer.pl
index 3272e52cae..2a035c2d0d 100644
--- a/src/test/authentication/t/003_peer.pl
+++ b/src/test/authentication/t/003_peer.pl
@@ -69,17 +69,6 @@ sub test_role
}
}
-# Find $pattern in log file of $node.
-sub find_in_log
-{
- my ($node, $offset, $pattern) = @_;
-
- my $log = PostgreSQL::Test::Utils::slurp_file($node->logfile, $offset);
- return 0 if (length($log) <= 0);
-
- return $log =~ m/$pattern/;
-}
-
my $node = PostgreSQL::Test::Cluster->new('node');
$node->init;
$node->append_conf('postgresql.conf', "log_connections = on\n");
@@ -91,9 +80,9 @@ reset_pg_hba($node, 'peer');
# Check if peer authentication is supported on this platform.
my $log_offset = -s $node->logfile;
$node->psql('postgres');
-if (find_in_log(
- $node, $log_offset,
- qr/peer authentication is not supported on this platform/))
+if ($node->log_contains(
+ qr/peer authentication is not supported on this platform/),
+ $log_offset,)
{
plan skip_all => 'peer authentication is not supported on this
platform';
}
diff --git a/src/test/perl/PostgreSQL/Test/Cluster.pm
b/src/test/perl/PostgreSQL/Test/Cluster.pm
index baea0fcd1c..4df7dd4dec 100644
--- a/src/test/perl/PostgreSQL/Test/Cluster.pm
+++ b/src/test/perl/PostgreSQL/Test/Cluster.pm
@@ -2536,6 +2536,21 @@ sub log_content
}
+=pod
+
+=item log_contains(pattern, offset)
+
+Find pattern in logfile of node after offset byte.
+
+=cut
+
+sub log_contains
+{
+ my ($self, $pattern, $offset) = @_;
+
+ return PostgreSQL::Test::Utils::slurp_file($self->logfile, $offset) =~
m/$pattern/;
+}
+
=pod
=item $node->run_log(...)
diff --git a/src/test/recovery/t/019_replslot_limit.pl
b/src/test/recovery/t/019_replslot_limit.pl
index a1aba16e14..95acf9e357 100644
--- a/src/test/recovery/t/019_replslot_limit.pl
+++ b/src/test/recovery/t/019_replslot_limit.pl
@@ -161,8 +161,7 @@ $node_primary->wait_for_catchup($node_standby);
$node_standby->stop;
-ok( !find_in_log(
- $node_standby,
+ok( !$node_standby->log_contains(
"requested WAL segment [0-9A-F]+ has already been removed"),
'check that required WAL segments are still available');
@@ -184,8 +183,8 @@ $node_primary->safe_psql('postgres', "CHECKPOINT;");
my $invalidated = 0;
for (my $i = 0; $i < 10 * $PostgreSQL::Test::Utils::timeout_default; $i++)
{
- if (find_in_log(
- $node_primary, 'invalidating obsolete replication slot
"rep1"',
+ if ($node_primary->log_contains(
+ 'invalidating obsolete replication slot "rep1"',
$logstart))
{
$invalidated = 1;
@@
Re: Prevent psql \watch from running queries that return no rows
On Fri, Jun 02, 2023 at 11:47:16AM -0400, Greg Sabino Mullane wrote: > * Not completely convinced of the name "zero" (better than > "stop_when_no_rows_returned"). Considered adding a new x=y argument, or > overloading c (c=-1) but neither seemed very intuitive. On the other hand, > it's tempting to stick to a single method moving forward, although this is > a boolean option not a x=y one like the other two. Wouldn't something like a target_rows be more flexible? You could use this parameter with a target number of rows to expect, zero being one choice in that. -- Michael signature.asc Description: PGP signature
Re: Docs: Encourage strong server verification with SCRAM
On Thu, Jun 01, 2023 at 08:28:32AM -0400, Michael Paquier wrote: > Hmm. Okay by me. Took me some time to get back to it, but applied this way. -- Michael signature.asc Description: PGP signature
Re: [BUG] pg_dump does not properly deal with BEGIN ATOMIC function
Kirk Wolak writes:
> On Fri, Jun 2, 2023 at 8:16 AM Tom Lane wrote:
>> BTW, now that I see a case the default printout here seems
>> completely ridiculous. I think we need to do
>> - pg_log_info(" %s", buf);
>> + pg_log_warning(" %s", buf);
> If I comprehend the suggestion, it will label each line with a warning.
> Which implies I have 6 Warnings.
Right, I'd forgotten that pg_log_warning() will interpose "warning:".
Attached are two more-carefully-thought-out suggestions. The easy
way is to use pg_log_warning_detail(), which produces output like
pg_dump: warning: could not resolve dependency loop among these items:
pg_dump: detail: FUNCTION a_f (ID 216 OID 40532)
pg_dump: detail: CONSTRAINT a_pkey (ID 3466 OID 40531)
pg_dump: detail: POST-DATA BOUNDARY (ID 3612)
pg_dump: detail: TABLE DATA a (ID 3610 OID 40525)
pg_dump: detail: PRE-DATA BOUNDARY (ID 3611)
Alternatively, we could assemble the details by hand, as in the
second patch, producing
pg_dump: warning: could not resolve dependency loop among these items:
FUNCTION a_f (ID 216 OID 40532)
CONSTRAINT a_pkey (ID 3466 OID 40531)
POST-DATA BOUNDARY (ID 3612)
TABLE DATA a (ID 3610 OID 40525)
PRE-DATA BOUNDARY (ID 3611)
I'm not really sure which of these I like better. The first one
is a much simpler code change, and there is some value in labeling
the output like that. The second patch's output seems less cluttered,
but it's committing a modularity sin by embedding formatting knowledge
at the caller level. Thoughts?
BTW, there is a similar abuse of pg_log_info just a few lines
above this, and probably others elsewhere. I won't bother
writing patches for other places till we have agreement on what
the output ought to look like.
regards, tom lane
diff --git a/src/bin/pg_dump/pg_dump_sort.c b/src/bin/pg_dump/pg_dump_sort.c
index 745578d855..3c66b8bf1a 100644
--- a/src/bin/pg_dump/pg_dump_sort.c
+++ b/src/bin/pg_dump/pg_dump_sort.c
@@ -1253,7 +1253,7 @@ repairDependencyLoop(DumpableObject **loop,
char buf[1024];
describeDumpableObject(loop[i], buf, sizeof(buf));
- pg_log_info(" %s", buf);
+ pg_log_warning_detail(" %s", buf);
}
if (nLoop > 1)
diff --git a/src/bin/pg_dump/pg_dump_sort.c b/src/bin/pg_dump/pg_dump_sort.c
index 745578d855..f0eef849d3 100644
--- a/src/bin/pg_dump/pg_dump_sort.c
+++ b/src/bin/pg_dump/pg_dump_sort.c
@@ -971,6 +971,7 @@ static void
repairDependencyLoop(DumpableObject **loop,
int nLoop)
{
+ PQExpBuffer msgbuf;
int i,
j;
@@ -1247,14 +1248,17 @@ repairDependencyLoop(DumpableObject **loop,
* If we can't find a principled way to break the loop, complain and break
* it in an arbitrary fashion.
*/
- pg_log_warning("could not resolve dependency loop among these items:");
+ msgbuf = createPQExpBuffer();
for (i = 0; i < nLoop; i++)
{
char buf[1024];
describeDumpableObject(loop[i], buf, sizeof(buf));
- pg_log_info(" %s", buf);
+ appendPQExpBuffer(msgbuf, "\n %s", buf);
}
+ pg_log_warning("could not resolve dependency loop among these items:%s",
+ msgbuf->data);
+ destroyPQExpBuffer(msgbuf);
if (nLoop > 1)
removeObjectDependency(loop[0], loop[1]->dumpId);
Improving FTS for Greek
I posted earlier in pgsql-general, that I realised there’s no greek.stop under
$(pg_config —sharedir)/tsearch_data
And indeed looks like stop words are maintained with to_tsvector(‘greek’, ..).
I wrote an extension https://github.com/Florents-Tselai/pg_fts_greek that adds
another ‘greek_ext’ regconfig
Here’s how the results compare
t to_tsvector('greek', t) to_tsvector('greek_ext', t)
'το τετράγωνο της υποτείνουσας ενός ορθογωνίου τριγώνου''εν':5
'ορθογων':6 'τ':3 'τετραγων':2 'το':1 'τριγων':7 'υποτεινουσ':4 'εν':5
'ορθογων':6 'τετραγων':2 'τριγων':7 'υποτεινουσ':4
'ο γιώργος είναι πονηρός' 'γιωργ':2 'εινα':3 'ο':1 'πονηρ':4
'γιωργ':2 'πονηρ':4
'ο ήλιος ο πράσινος o ήλιος που ανατέλλει' 'o':5 'ανατελλ':8 'ηλι':2,6
'ο':1,3 'π':7 'πρασιν':4'ανατελλ':8 'ηλι':2,6 'πρασιν':4
There’s another previous relevant patch [0] but was never merged. I’ve included
these stop words and added some more (info in README.md).
For my personal projects looks like it yields much better results.
I’d like some feedback on the extension ; particularly on the installation
infra (I’m not sure I’ve handled properly the permissions in the .sql files)
I’ll then try to make a .patch for this.
[0]
https://www.postgresql.org/message-id/flat/e1c79330-48a5-abef-c309-8d4499e3180b%402ndquadrant.com#7431fdb9ae24b694155aef3f040b7b60
Improve join_search_one_level readibilty (one line change)
Hello hackers
Attached is my first patch for PostgreSQL, which is a simple one-liner
that I believe can improve the code.
In the "join_search_one_level" function, I noticed that the variable
"other_rels_list" always refers to "joinrels[1]" even when the (level
== 2) condition is met. I propose changing:
"other_rels_list = joinrels[level - 1]" to "other_rels_list = joinrels[1]"
This modification aims to enhance clarity and avoid unnecessary instructions.
I would greatly appreciate any review and feedback on this patch as I
am a newcomer to PostgreSQL contributions. Your input will help me
improve and contribute effectively to the project.
I have followed the excellent guide "How to submit a patch by email,
2023 edition" by Peter Eisentraut.
Additionally, if anyone has any tips on ensuring that Gmail recognizes
my attached patches as the "text/x-patch" MIME type when sending them
from the Chrome client, I would be grateful for the advice.
Or maybe the best practice is to use Git send-email ?
Thank you for your time.
Best regards
Alex Hsieh
From 18d2ed81d3640881082bf37d6e3021ffe671d215 Mon Sep 17 00:00:00 2001
From: DouEnergy
Date: Fri, 2 Jun 2023 20:58:07 +0800
Subject: [PATCH] Improve left deep tree dp algorithm's readability
---
src/backend/optimizer/path/joinrels.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/backend/optimizer/path/joinrels.c b/src/backend/optimizer/path/joinrels.c
index 2feab2184f..bca78d07fa 100644
--- a/src/backend/optimizer/path/joinrels.c
+++ b/src/backend/optimizer/path/joinrels.c
@@ -111,7 +111,7 @@ join_search_one_level(PlannerInfo *root, int level)
if (level == 2) /* consider remaining initial rels */
{
-other_rels_list = joinrels[level - 1];
+other_rels_list = joinrels[1];
other_rels = lnext(other_rels_list, r);
}
else/* consider all initial rels */
--
2.37.1 (Apple Git-137.1)
Re: Should "REGRESS_OPTS = --temp-config" be working for 3rd party extensions?
Hi Julien, > temp-config can only be used when bootstrapping a temporary environment, so > when using e.g. make check. PGXS / third-party extension can only use > installcheck, so if you need specific config like shared_preload_libraries you > need to manually configure your instance beforehand, or indeed rely on TAP > tests. Most projects properly setup their instance in the CI jobs, and at > least the Debian packaging infrastructure has a way to configure it too. Many thanks! -- Best regards, Aleksander Alekseev
Re: Should "REGRESS_OPTS = --temp-config" be working for 3rd party extensions?
On Sat, Jun 03, 2023 at 02:56:27PM +0300, Aleksander Alekseev wrote: > > I tried to use `REGRESS_OPTS = --temp-config` in order to test a 3rd > party extension with a custom .conf file similarly to how PostgreSQL > does it for src/test/modules/test_slru. It didn't work and "38.18. > Extension Building Infrastructure" [1] doesn't seem to be much help. > > Is it accurate to say that the author of a 3rd party extension that > uses shared_preload_libraries can't be using SQL tests and has to use > TAP tests instead? If not then what did I miss? temp-config can only be used when bootstrapping a temporary environment, so when using e.g. make check. PGXS / third-party extension can only use installcheck, so if you need specific config like shared_preload_libraries you need to manually configure your instance beforehand, or indeed rely on TAP tests. Most projects properly setup their instance in the CI jobs, and at least the Debian packaging infrastructure has a way to configure it too.
Re: Avoid unused value (src/fe_utils/print.c)
Em sáb., 3 de jun. de 2023 às 09:00, Alexander Lakhin escreveu: > Hello Ranier, > > 03.06.2023 13:14, Ranier Vilela wrote: > > Hi, > > > > This is for Postgres 17 (head). > > > > Per Coverity. > > At function print_unaligned_text, variable "need_recordsep", is > > unnecessarily set to true and false. > > Clang' scan-build detects 58 errors "Dead assignment", including that one. > Maybe it would be more sensible to eliminate all errors of this class? > Hi Alexander, Sure. I hope that when you or I are a committer, we can fix a whole class of bugs together. best regards, Ranier Vilela
Re: [PATCH] Slight improvement of worker_spi.c example
Hi, > That being said this module is really naive and has so many problems that I > don't think it's actually helpful as coding guidelines for anyone who wants to > create a non toy extension using bgworkers. Agree. It is a simple example and I don't think it's going to be useful to make a complicated one out of it. The order of the calls it currently uses however may be extremely confusing for newcomers. It creates an impression that this particular order is extremely important while in fact it's not and it takes time to figure this out. -- Best regards, Aleksander Alekseev
Re: [PATCH] Slight improvement of worker_spi.c example
On Sat, Jun 03, 2023 at 02:38:26PM +0300, Aleksander Alekseev wrote: > Hi Julien, > > > I'm pretty sure that this is intentional. The worker can be launched > > dynamically and in that case it still needs a GUC for the naptime. > > The dynamic worker also is going to need worker_spi_database, however > the corresponding GUC declaration is placed below the check. Yes, and that's because that GUC is declared as PGC_POSTMASTER so you can't declare such a GUC dynamically. > > Perhaps we should just say that the extension shouldn't be used > without shared_preload_libraies. We are not testing whether it works > in such a case anyway. The patch that added the database name clearly was committed without bothering testing that scenario, but it would be better to fix it and add some coverage rather than remove some example of how to use dynamic bgworkers. Maybe with a assign hook to make sure it's never changed once assigned or something along those lines. That being said this module is really naive and has so many problems that I don't think it's actually helpful as coding guidelines for anyone who wants to create a non toy extension using bgworkers.
Re: PostgreSQL 16 Beta 1 Released!
Marco Atzeri writes: > just me ? No. https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=5df5bea29070b420452bdb257c3dec1cf0419fca regards, tom lane
Re: Avoid unused value (src/fe_utils/print.c)
Hello Ranier, 03.06.2023 13:14, Ranier Vilela wrote: Hi, This is for Postgres 17 (head). Per Coverity. At function print_unaligned_text, variable "need_recordsep", is unnecessarily set to true and false. Clang' scan-build detects 58 errors "Dead assignment", including that one. Maybe it would be more sensible to eliminate all errors of this class? Best regards, Alexander
Should "REGRESS_OPTS = --temp-config" be working for 3rd party extensions?
Hi, I tried to use `REGRESS_OPTS = --temp-config` in order to test a 3rd party extension with a custom .conf file similarly to how PostgreSQL does it for src/test/modules/test_slru. It didn't work and "38.18. Extension Building Infrastructure" [1] doesn't seem to be much help. Here is my Makefile: ``` EXTENSION = experiment MODULES = experiment DATA = experiment--1.0.sql experiment.conf REGRESS_OPTS = --temp-config $(top_srcdir)/../../../share/postgresql/extension/experiment.conf REGRESS = experiment PG_CPPFLAGS = -g -O0 SHLIB_LINK = ifndef PG_CONFIG PG_CONFIG := pg_config endif PGXS := $(shell $(PG_CONFIG) --pgxs) include $(PGXS) ``` And the result I get: ``` $ make clean && make install && make installcheck ... # note: experiment.conf is copied according to DATA value: /bin/sh /Users/eax/pginstall/lib/postgresql/pgxs/src/makefiles/../../config/install-sh -c -m 644 .//experiment--1.0.sql .//experiment.conf '/Users/eax/pginstall/share/postgresql/extension/' # note: --temp-conf path is correct echo "# +++ regress install-check in +++" && /Users/eax/pginstall/lib/postgresql/pgxs/src/makefiles/../../src/test/regress/pg_regress --inputdir=./ --bindir='/Users/eax/pginstall/bin'--temp-config /Users/eax/pginstall/lib/postgresql/pgxs/src/makefiles/../../../../../share/postgresql/extension/experiment.conf --dbname=contrib_regression experiment # +++ regress install-check in +++ # using postmaster on Unix socket, default port not ok 1 - experiment382 ms # note: shared_preload_libraries had no effect and I got elog() from the extension: $ cat /Users/eax/projects/c/postgresql-extensions/007-gucs/regression.diffs ... +FATAL: Please use shared_preload_libraries ``` This comment in Makefile for test_slru seems to explain why this happens: ``` # Disabled because these tests require "shared_preload_libraries=test_slru", # which typical installcheck users do not have (e.g. buildfarm clients). NO_INSTALLCHECK = 1 ``` The complete example is available on GitHub [2]. Is it accurate to say that the author of a 3rd party extension that uses shared_preload_libraries can't be using SQL tests and has to use TAP tests instead? If not then what did I miss? [1]: https://www.postgresql.org/docs/current/extend-pgxs.html [2]: https://github.com/afiskon/postgresql-extensions/tree/temp_config_experiment/007-gucs -- Best regards, Aleksander Alekseev
Re: [PATCH] Slight improvement of worker_spi.c example
Hi Julien, > I'm pretty sure that this is intentional. The worker can be launched > dynamically and in that case it still needs a GUC for the naptime. The dynamic worker also is going to need worker_spi_database, however the corresponding GUC declaration is placed below the check. Perhaps we should just say that the extension shouldn't be used without shared_preload_libraies. We are not testing whether it works in such a case anyway. -- Best regards, Aleksander Alekseev
Re: [PATCH] Slight improvement of worker_spi.c example
On Sat, Jun 03, 2023 at 02:09:26PM +0300, Aleksander Alekseev wrote: > > Additionally I noticed that the check: > > ``` > if (!process_shared_preload_libraries_in_progress) > return; > ``` > > ... was misplaced in _PG_init(). Here is the patch v2 which fixes this too. I'm pretty sure that this is intentional. The worker can be launched dynamically and in that case it still needs a GUC for the naptime.
Re: [PATCH] Slight improvement of worker_spi.c example
Hi,
> The patch changes the order to:
>
> StartTransactionCommand();
> PushActiveSnapshot(...);
> SPI_connect();
>
> ...
>
> SPI_finish();
> PopActiveSnapshot();
> CommitTransactionCommand();
>
> ... and also clarifies that the order of PushActiveSnapshot(...) and
> SPI_connect() is not important.
Additionally I noticed that the check:
```
if (!process_shared_preload_libraries_in_progress)
return;
```
... was misplaced in _PG_init(). Here is the patch v2 which fixes this too.
--
Best regards,
Aleksander Alekseev
From 4bf425356897a0c47060bb877d88049963fda814 Mon Sep 17 00:00:00 2001
From: Aleksander Alekseev
Date: Fri, 12 May 2023 17:15:04 +0300
Subject: [PATCH v2] Slight improvement of worker_spi.c example
Previously the example used the following order of calls:
StartTransactionCommand();
SPI_connect();
PushActiveSnapshot(...);
...
SPI_finish();
PopActiveSnapshot();
CommitTransactionCommand();
This could be somewhat misleading. Typically one expects something to be freed
in a reverse order comparing to initialization. This creates a false impression
that PushActiveSnapshot(...) _should_ be called after SPI_connect().
The patch changes the order to:
StartTransactionCommand();
PushActiveSnapshot(...);
SPI_connect();
...
SPI_finish();
PopActiveSnapshot();
CommitTransactionCommand();
... and also clarifies that the order of PushActiveSnapshot(...) and
SPI_connect() is not important.
Additionally move the check:
if (!process_shared_preload_libraries_in_progress)
return;
... to the beginning of _PG_init(). The check was previously misplaced.
Aleksander Alekseev, reviewed by TODO FIXME
Discussion: CAJ7c6TMOhWmP92_fFss-cvNnsxdj5_TSdtmiE3AJOv6yGotoFQ@mail.gmail.com">https://postgr.es/m/CAJ7c6TMOhWmP92_fFss-cvNnsxdj5_TSdtmiE3AJOv6yGotoFQ@mail.gmail.com
---
src/test/modules/worker_spi/worker_spi.c | 16 +---
1 file changed, 9 insertions(+), 7 deletions(-)
diff --git a/src/test/modules/worker_spi/worker_spi.c b/src/test/modules/worker_spi/worker_spi.c
index ad491d7722..7a363bbe11 100644
--- a/src/test/modules/worker_spi/worker_spi.c
+++ b/src/test/modules/worker_spi/worker_spi.c
@@ -74,8 +74,8 @@ initialize_worker_spi(worktable *table)
SetCurrentStatementStartTimestamp();
StartTransactionCommand();
- SPI_connect();
PushActiveSnapshot(GetTransactionSnapshot());
+ SPI_connect();
pgstat_report_activity(STATE_RUNNING, "initializing worker_spi schema");
/* XXX could we use CREATE SCHEMA IF NOT EXISTS? */
@@ -222,17 +222,19 @@ worker_spi_main(Datum main_arg)
* preceded by SetCurrentStatementStartTimestamp(), so that statement
* start time is always up to date.
*
- * The SPI_connect() call lets us run queries through the SPI manager,
- * and the PushActiveSnapshot() call creates an "active" snapshot
+ * The PushActiveSnapshot() call creates an "active" snapshot,
* which is necessary for queries to have MVCC data to work on.
+ * The SPI_connect() call lets us run queries through the SPI manager.
+ * The order of PushActiveSnapshot() and SPI_connect() is not really
+ * important.
*
* The pgstat_report_activity() call makes our activity visible
* through the pgstat views.
*/
SetCurrentStatementStartTimestamp();
StartTransactionCommand();
- SPI_connect();
PushActiveSnapshot(GetTransactionSnapshot());
+ SPI_connect();
debug_query_string = buf.data;
pgstat_report_activity(STATE_RUNNING, buf.data);
@@ -282,6 +284,9 @@ _PG_init(void)
{
BackgroundWorker worker;
+ if (!process_shared_preload_libraries_in_progress)
+ return;
+
/* get the configuration */
DefineCustomIntVariable("worker_spi.naptime",
"Duration between each check (in seconds).",
@@ -296,9 +301,6 @@ _PG_init(void)
NULL,
NULL);
- if (!process_shared_preload_libraries_in_progress)
- return;
-
DefineCustomIntVariable("worker_spi.total_workers",
"Number of workers.",
NULL,
--
2.40.1
Avoid unused value (src/fe_utils/print.c)
Hi, This is for Postgres 17 (head). Per Coverity. At function print_unaligned_text, variable "need_recordsep", is unnecessarily set to true and false. Attached a trivial fix patch. regards, Ranier Vilela avoid-unused-value-print.patch Description: Binary data
