SELECT Query taking 200 ms on PostgreSQL compared to 4 ms on Oracle after migration.

On Sat, Apr 3, 2021 at 7:08 PM aditya desai <admad123@gmail.com> wrote:

Hi,
We migrated our Oracle Databases to PostgreSQL. One of the simple select query that takes 4 ms on Oracle is taking around 200 ms on PostgreSQL. Could you please advise. Please find query and query plans below. Gather cost seems high. Will increasing max_parallel_worker_per_gather help?

No it doesn't. For small tables, parallelism might not help since it
doesn't come for free. Try setting max_parallel_worker_per_gather to 0
i.e. without parallel query.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

so 3. 4. 2021 v 15:38 odesílatel aditya desai <admad123@gmail.com> napsal:

Hi,
We migrated our Oracle Databases to PostgreSQL. One of the simple select
query that takes 4 ms on Oracle is taking around 200 ms on PostgreSQL.
Could you please advise. Please find query and query plans below. Gather
cost seems high. Will increasing max_parallel_worker_per_gather help?

explain analyse SELECT bom.address_key dom2137,bom.address_type_key
dom1727,bom.start_date dom1077,bom.end_date dom828,bom.address_status_key
dom1955,bom.address_role_key dom1711,bom.delivery_point_created
dom2362,bom.postcode dom873,bom.postcode_name dom1390,bom.street_name
dom1186,bom.premises_number_1 dom1777,bom.premises_number_2
dom1778,bom.premises_letter_1 dom1784,bom.premises_letter_2
dom1785,bom.premises_separator dom1962,bom.stairway dom892,bom.po_box
dom653,bom.apartment_number dom1732,bom.apartment_letter
dom1739,bom.street_key dom1097,bom.address_use_key dom1609,bom.language_key
dom1272,bom.address_family_id dom1796,bom.cur_address_key
dom2566,bom.created_by dom1052,bom.modified_by dom1158,bom.creation_time
dom1392,bom.modification_time dom1813 FROM DEPT.address dom WHERE
address_key = 6113763

[
{
"Plan": {
"Node Type": "Gather",
"Parallel Aware": false,
"Actual Rows": 1,
"Actual Loops": 1,
"Workers Planned": 1,
"Workers Launched": 1,
"Single Copy": true,
"Plans": [
{
"Node Type": "Index Scan",
"Parent Relationship": "Outer",
"Parallel Aware": false,
"Scan Direction": "Forward",
"Index Name": "address1_i7",
"Relation Name": "address",
"Alias": "dom",
"Actual Rows": 1,
"Actual Loops": 1,
"Index Cond": "(address_key = 6113763)",
"Rows Removed by Index Recheck": 0
}
]
},
"Triggers": []
}
]

"Gather (cost=1000.43..1002.75 rows=1 width=127) (actual
time=174.318..198.539 rows=1 loops=1)"
" Workers Planned: 1"
" Workers Launched: 1"
" Single Copy: true"
" -> Index Scan using address1_i7 on address1 dom (cost=0.43..2.65 rows=1
width=127) (actual time=0.125..0.125 rows=1 loops=1)"
" Index Cond: (address_key = 6113763)"
"Planning Time: 0.221 ms"
"Execution Time: 198.601 ms"

You should have broken configuration - there is not any reason to start
parallelism - probably some option in postgresql.conf has very bad value.
Second - it's crazy to see 200 ms just on interprocess communication -
maybe your CPU is overutilized.

Regards

Pavel

It seems like something is missing. Is this table partitioned? How long ago
was migration done? Has vacuum freeze and analyze of tables been done? Was
index created after populating data or reindexed after perhaps? What
version are you using?

On Sat, Apr 03, 2021 at 04:08:01PM +0200, Pavel Stehule wrote:

so 3. 4. 2021 v 15:38 odes�latel aditya desai <admad123@gmail.com> napsal:

"Gather (cost=1000.43..1002.75 rows=1 width=127) (actual
time=174.318..198.539 rows=1 loops=1)"
" Workers Planned: 1"
" Workers Launched: 1"
" Single Copy: true"
" -> Index Scan using address1_i7 on address1 dom (cost=0.43..2.65 rows=1
width=127) (actual time=0.125..0.125 rows=1 loops=1)"
" Index Cond: (address_key = 6113763)"
"Planning Time: 0.221 ms"
"Execution Time: 198.601 ms"

You should have broken configuration - there is not any reason to start
parallelism - probably some option in postgresql.conf has very bad value.
Second - it's crazy to see 200 ms just on interprocess communication -
maybe your CPU is overutilized.

It seems like force_parallel_mode is set, which is for debugging and not for
"forcing things to go faster". Maybe we should rename the parameter, like
parallel_mode_testing=on.

http://rhaas.blogspot.com/2018/06/using-forceparallelmode-correctly.html

--
Justin

Hi Michael,
Thanks for your response.
Is this table partitioned? - No
How long ago was migration done? - 27th March 2021
Has vacuum freeze and analyze of tables been done? - We ran vacuum analyze.
Was index created after populating data or reindexed after perhaps? -
Index was created after data load and reindex was executed on all tables
yesterday.
Version is PostgreSQL-11

Regards,
Aditya.

On Sat, Apr 3, 2021 at 7:40 PM Michael Lewis <mlewis@entrata.com> wrote:

On Sat, Apr 3, 2021 at 08:29:22PM +0530, aditya desai wrote:

Hi Michael,
Thanks for your response.
Is this table partitioned? - No
How long ago was migration done? - 27th March 2021
Has vacuum freeze and analyze of tables been done? - We ran vacuum analyze.
�Was index created after populating data or reindexed after perhaps? - Index
was created after data load and reindex was executed on all tables yesterday.
�Version is PostgreSQL-11

FYI, the output of these queries will show u what changes have been made
to the configuration file:

SELECT version();

SELECT name, current_setting(name), source
FROM pg_settings
WHERE source NOT IN ('default', 'override');

--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com

If only the physical world exists, free will is an illusion.

Hi Justin,
Yes, force_parallel_mode is on. Should we set it off?

Regards,
Aditya.

On Sat, Apr 3, 2021 at 7:46 PM Justin Pryzby <pryzby@telsasoft.com> wrote:

On Sat, Apr 3, 2021 at 08:38:18PM +0530, aditya desai wrote:

Hi Justin,
Yes, force_parallel_mode is on. Should we set it off?

Yes. I bet someone set it without reading our docs:

https://www.postgresql.org/docs/13/runtime-config-query.html#RUNTIME-CONFIG-QUERY-OTHER

--> Allows the use of parallel queries for testing purposes even in cases
--> where no performance benefit is expected.

We might need to clarify this sentence to be clearer it is _only_ for
testing. Also, I suggest you review _all_ changes that have been made
to the server since I am worried other unwise changes might also have
been made.

---------------------------------------------------------------------------

Regards,
Aditya.

On Sat, Apr 3, 2021 at 7:46 PM Justin Pryzby <pryzby@telsasoft.com> wrote:

On Sat, Apr 03, 2021 at 04:08:01PM +0200, Pavel Stehule wrote:

so 3. 4. 2021 v 15:38 odes�latel aditya desai <admad123@gmail.com>

napsal:

"Gather (cost=1000.43..1002.75 rows=1 width=127) (actual
time=174.318..198.539 rows=1 loops=1)"
" Workers Planned: 1"
" Workers Launched: 1"
" Single Copy: true"
" -> Index Scan using address1_i7 on address1 dom (cost=0.43..2.65 rows

=1

width=127) (actual time=0.125..0.125 rows=1 loops=1)"
" Index Cond: (address_key = 6113763)"
"Planning Time: 0.221 ms"
"Execution Time: 198.601 ms"

You should have broken configuration - there is not any reason to start
parallelism -� probably some option in postgresql.conf has very bad

value.

Second - it's crazy to see 200 ms just on interprocess communication -
maybe your CPU is overutilized.

It seems like force_parallel_mode is set, which is for debugging and not
for
"forcing things to go faster".� Maybe we should rename the parameter, like
parallel_mode_testing=on.

http://rhaas.blogspot.com/2018/06/using-forceparallelmode-correctly.html

--
Justin

--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com

If only the physical world exists, free will is an illusion.

Thanks Bruce!! Will set it off and retry.

On Sat, Apr 3, 2021 at 8:42 PM Bruce Momjian <bruce@momjian.us> wrote:

On Sat, Apr 3, 2021 at 11:12:01AM -0400, Bruce Momjian wrote:

On Sat, Apr 3, 2021 at 08:38:18PM +0530, aditya desai wrote:

Hi Justin,
Yes, force_parallel_mode is on. Should we set it off?

Yes. I bet someone set it without reading our docs:

https://www.postgresql.org/docs/13/runtime-config-query.html#RUNTIME-CONFIG-QUERY-OTHER

--> Allows the use of parallel queries for testing purposes even in cases
--> where no performance benefit is expected.

We might need to clarify this sentence to be clearer it is _only_ for
testing. Also, I suggest you review _all_ changes that have been made
to the server since I am worried other unwise changes might also have
been made.

This brings up an issue we see occasionally. You can either leave
everything as default, get advice on which defaults to change, or study
each setting and then change defaults. Changing defaults without study
often leads to poor configurations, as we are seeing here.

The lucky thing is that you noticed a slow query and found the
misconfiguration, but I am sure there are many servers where
misconfiguration is never detected. I wish I knew how to improve this
situation, but user education seems to be all we can do.

--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com

If only the physical world exists, free will is an illusion.

adding the group.

aad_log_min_messages | warning
| configuration file
application_name | psql
| client
archive_command |
c:\postgres\bin\xlogcopy\xlogcopy.exe archive blob "%f" "%p" |
configuration file
archive_mode | on
| configuration file
archive_timeout | 15min
| configuration file
authentication_timeout | 30s
| configuration file
autovacuum_analyze_scale_factor | 0.05
| configuration file
autovacuum_naptime | 15s
| configuration file
autovacuum_vacuum_scale_factor | 0.05
| configuration file
bgwriter_delay | 20ms
| configuration file
bgwriter_flush_after | 512kB
| configuration file
bgwriter_lru_maxpages | 100
| configuration file
checkpoint_completion_target | 0.9
| configuration file
checkpoint_flush_after | 256kB
| configuration file
checkpoint_timeout | 5min
| configuration file
client_encoding | UTF8
| client
connection_ID |
5b59f092-444c-49df-b5d6-a7a0028a7855 | client
connection_PeerIP |
fd40:4d4a:11:5067:6d11:500:a07:5144 | client
connection_Vnet | on
| client
constraint_exclusion | partition
| configuration file
data_sync_retry | on
| configuration file
DateStyle | ISO, MDY
| configuration file
default_text_search_config | pg_catalog.english
| configuration file
dynamic_shared_memory_type | windows
| configuration file
effective_cache_size | 160GB
| configuration file
enable_seqscan | off
| configuration file
force_parallel_mode | off
| configuration file
from_collapse_limit | 15
| configuration file
full_page_writes | off
| configuration file
hot_standby | on
| configuration file
hot_standby_feedback | on
| configuration file
join_collapse_limit | 15
| configuration file
lc_messages | English_United States.1252
| configuration file
lc_monetary | English_United States.1252
| configuration file
lc_numeric | English_United States.1252
| configuration file
lc_time | English_United States.1252
| configuration file
listen_addresses | *
| configuration file
log_checkpoints | on
| configuration file
log_connections | on
| configuration file
log_destination | stderr
| configuration file
log_file_mode | 0640
| configuration file
log_line_prefix | %t-%c-
| configuration file
log_min_messages_internal | info
| configuration file
log_rotation_age | 1h
| configuration file
log_rotation_size | 100MB
| configuration file
log_timezone | UTC
| configuration file
logging_collector | on
| configuration file
maintenance_work_mem | 1GB
| configuration file
max_connections | 1900
| configuration file
max_parallel_workers_per_gather | 16
| configuration file
max_replication_slots | 10
| configuration file
max_stack_depth | 2MB
| environment variable
max_wal_senders | 10
| configuration file
max_wal_size | 26931MB
| configuration file
min_wal_size | 4GB
| configuration file
pg_qs.query_capture_mode | top
| configuration file
pgms_wait_sampling.query_capture_mode | all
| configuration file
pgstat_udp_port | 20224
| command line
port | 20224
| command line
random_page_cost | 1.1
| configuration file
shared_buffers | 64GB
| configuration file
ssl | on
| configuration file
ssl_ca_file | root.crt
| configuration file
superuser_reserved_connections | 5
| configuration file
TimeZone | EET
| configuration file
track_io_timing | on
| configuration file
wal_buffers | 128MB
| configuration file
wal_keep_segments | 25
| configuration file
wal_level | replica
| configuration file
work_mem | 16MB
| configuration file

On Sat, Apr 3, 2021 at 8:59 PM aditya desai <admad123@gmail.com> wrote:

I will gather all information and get back to you

On Sat, Apr 3, 2021 at 9:00 PM Pavel Stehule <pavel.stehule@gmail.com>
wrote:

so 3. 4. 2021 v 17:30 odesílatel aditya desai <admad123@gmail.com> napsal:

adding the group.

aad_log_min_messages | warning
| configuration file
application_name | psql
| client
archive_command |
c:\postgres\bin\xlogcopy\xlogcopy.exe archive blob "%f" "%p" |
configuration file
archive_mode | on
| configuration file
archive_timeout | 15min
| configuration file
authentication_timeout | 30s
| configuration file
autovacuum_analyze_scale_factor | 0.05
| configuration file
autovacuum_naptime | 15s
| configuration file
autovacuum_vacuum_scale_factor | 0.05
| configuration file
bgwriter_delay | 20ms
| configuration file
bgwriter_flush_after | 512kB
| configuration file
bgwriter_lru_maxpages | 100
| configuration file
checkpoint_completion_target | 0.9
| configuration file
checkpoint_flush_after | 256kB
| configuration file
checkpoint_timeout | 5min
| configuration file
client_encoding | UTF8
| client
connection_ID |
5b59f092-444c-49df-b5d6-a7a0028a7855 | client
connection_PeerIP |
fd40:4d4a:11:5067:6d11:500:a07:5144 | client
connection_Vnet | on
| client
constraint_exclusion | partition
| configuration file
data_sync_retry | on
| configuration file
DateStyle | ISO, MDY
| configuration file
default_text_search_config | pg_catalog.english
| configuration file
dynamic_shared_memory_type | windows
| configuration file
effective_cache_size | 160GB
| configuration file
enable_seqscan | off
| configuration file
force_parallel_mode | off
| configuration file
from_collapse_limit | 15
| configuration file
full_page_writes | off
| configuration file
hot_standby | on
| configuration file
hot_standby_feedback | on
| configuration file
join_collapse_limit | 15
| configuration file
lc_messages | English_United States.1252
| configuration file
lc_monetary | English_United States.1252
| configuration file
lc_numeric | English_United States.1252
| configuration file
lc_time | English_United States.1252
| configuration file
listen_addresses | *
| configuration file
log_checkpoints | on
| configuration file
log_connections | on
| configuration file
log_destination | stderr
| configuration file
log_file_mode | 0640
| configuration file
log_line_prefix | %t-%c-
| configuration file
log_min_messages_internal | info
| configuration file
log_rotation_age | 1h
| configuration file
log_rotation_size | 100MB
| configuration file
log_timezone | UTC
| configuration file
logging_collector | on
| configuration file
maintenance_work_mem | 1GB
| configuration file
max_connections | 1900
| configuration file
max_parallel_workers_per_gather | 16
| configuration file
max_replication_slots | 10
| configuration file
max_stack_depth | 2MB
| environment variable
max_wal_senders | 10
| configuration file
max_wal_size | 26931MB
| configuration file
min_wal_size | 4GB
| configuration file
pg_qs.query_capture_mode | top
| configuration file
pgms_wait_sampling.query_capture_mode | all
| configuration file
pgstat_udp_port | 20224
| command line
port | 20224
| command line
random_page_cost | 1.1
| configuration file
shared_buffers | 64GB
| configuration file
ssl | on
| configuration file
ssl_ca_file | root.crt
| configuration file
superuser_reserved_connections | 5
| configuration file
TimeZone | EET
| configuration file
track_io_timing | on
| configuration file
wal_buffers | 128MB
| configuration file
wal_keep_segments | 25
| configuration file
wal_level | replica
| configuration file
work_mem | 16MB
| configuration file

max_connections | 1900

it is really not good - there can be very high CPU overloading with a lot
of others issues.

On Sat, Apr 3, 2021 at 09:00:24PM +0530, aditya desai wrote:

adding the group.

Perfect. That is a lot of non-default settings, so I would be concerned
there are other misconfigurations in there --- the group here might have
some tips.

�aad_log_min_messages� � � � � � � � � | warning� � � � � � � � � � � � � � � �
� � � � � � � � � � � | configuration file

The above is not a PG config variable.

�connection_ID� � � � � � � � � � � � �| 5b59f092-444c-49df-b5d6-a7a0028a7855�
� � � � � � � � � � � �| client
�connection_PeerIP� � � � � � � � � � �| fd40:4d4a:11:5067:6d11:500:a07:5144� �
� � � � � � � � � � � | client
�connection_Vnet� � � � � � � � � � � �| on� � � � � � � � � � � � � � � � � �

Uh, these are not a PG settings. You need to show us the output of
version() because this is not standard Postgres. A quick search
suggests this is a Microsoft version of Postgres. I will stop
commenting.

--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com

If only the physical world exists, free will is an illusion.

Bruce Momjian <bruce@momjian.us> writes:

On Sat, Apr 3, 2021 at 08:38:18PM +0530, aditya desai wrote:

Yes, force_parallel_mode is on. Should we set it off?

Yes. I bet someone set it without reading our docs:

https://www.postgresql.org/docs/13/runtime-config-query.html#RUNTIME-CONFIG-QUERY-OTHER

--> Allows the use of parallel queries for testing purposes even in cases
--> where no performance benefit is expected.

We might need to clarify this sentence to be clearer it is _only_ for
testing.

I wonder why it is listed under planner options at all, and not under
developer options.

regards, tom lane

On Sat, Apr 03, 2021 at 11:39:19AM -0400, Tom Lane wrote:

Bruce Momjian <bruce@momjian.us> writes:

On Sat, Apr 3, 2021 at 08:38:18PM +0530, aditya desai wrote:

Yes, force_parallel_mode is on. Should we set it off?

Yes. I bet someone set it without reading our docs:

https://www.postgresql.org/docs/13/runtime-config-query.html#RUNTIME-CONFIG-QUERY-OTHER

--> Allows the use of parallel queries for testing purposes even in cases
--> where no performance benefit is expected.

We might need to clarify this sentence to be clearer it is _only_ for
testing.

I wonder why it is listed under planner options at all, and not under
developer options.

Because it's there to help DBAs catch errors in functions incorrectly marked as
parallel safe.

--
Justin

On Sat, Apr 3, 2021 at 11:39:19AM -0400, Tom Lane wrote:

Bruce Momjian <bruce@momjian.us> writes:

On Sat, Apr 3, 2021 at 08:38:18PM +0530, aditya desai wrote:

Yes, force_parallel_mode is on. Should we set it off?

Yes. I bet someone set it without reading our docs:

https://www.postgresql.org/docs/13/runtime-config-query.html#RUNTIME-CONFIG-QUERY-OTHER

--> Allows the use of parallel queries for testing purposes even in cases
--> where no performance benefit is expected.

We might need to clarify this sentence to be clearer it is _only_ for
testing.

I wonder why it is listed under planner options at all, and not under
developer options.

I was kind of surprised by that myself since I was working on a blog
entry about from_collapse_limit and join_collapse_limit. I think moving
it makes sense.

--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com

If only the physical world exists, free will is an illusion.

On Sat, Apr 3, 2021 at 10:41:14AM -0500, Justin Pryzby wrote:

On Sat, Apr 03, 2021 at 11:39:19AM -0400, Tom Lane wrote:

Bruce Momjian <bruce@momjian.us> writes:

On Sat, Apr 3, 2021 at 08:38:18PM +0530, aditya desai wrote:

Yes, force_parallel_mode is on. Should we set it off?

Yes. I bet someone set it without reading our docs:

https://www.postgresql.org/docs/13/runtime-config-query.html#RUNTIME-CONFIG-QUERY-OTHER

--> Allows the use of parallel queries for testing purposes even in cases
--> where no performance benefit is expected.

We might need to clarify this sentence to be clearer it is _only_ for
testing.

I wonder why it is listed under planner options at all, and not under
developer options.

Because it's there to help DBAs catch errors in functions incorrectly marked as
parallel safe.

Uh, isn't that developer/debugging?

--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com

If only the physical world exists, free will is an illusion.

Thanks Justin. Will review all parameters and get back to you.

On Sat, Apr 3, 2021 at 9:11 PM Justin Pryzby <pryzby@telsasoft.com> wrote:

Hi Justin/Bruce/Pavel,
Thanks for your inputs. After setting force_parallel_mode=off Execution
time of same query was reduced to 1ms from 200 ms. Worked like a charm. We
also increased work_mem to 80=MB. Thanks again.

Regards,
Aditya.

On Sat, Apr 3, 2021 at 9:14 PM aditya desai <admad123@gmail.com> wrote:

so 3. 4. 2021 v 19:37 odesílatel aditya desai <admad123@gmail.com> napsal:

Hi Justin/Bruce/Pavel,
Thanks for your inputs. After setting force_parallel_mode=off Execution
time of same query was reduced to 1ms from 200 ms. Worked like a charm. We
also increased work_mem to 80=MB. Thanks

super.

The too big max_connection can cause a lot of problems. You should install
and use pgbouncer or pgpool II.

https://scalegrid.io/blog/postgresql-connection-pooling-part-4-pgbouncer-vs-pgpool/

Regards

Pavel

Yes. I have made suggestions on connection pooling as well. Currently it is
being done from Application side.

On Sat, Apr 3, 2021 at 11:12 PM Pavel Stehule <pavel.stehule@gmail.com>
wrote:

so 3. 4. 2021 v 19:45 odesílatel aditya desai <admad123@gmail.com> napsal:

Yes. I have made suggestions on connection pooling as well. Currently it
is being done from Application side.

It is usual - but the application side pooling doesn't solve well
overloading. The behaviour of the database is not linear. Usually opened
connections are not active. But any non active connection can be changed to
an active connection (there is not any limit for active connections), and
then the performance can be very very slow. Good pooling and good setting
of max_connections is protection against overloading. max_connection should
be 10-20 x CPU cores (for OLTP)

Regards

Pavel

Forking this thread
/messages/by-id/20210403154336.GG29125@momjian.us

On Sat, Apr 3, 2021 at 08:38:18PM +0530, aditya desai wrote:

Yes, force_parallel_mode is on. Should we set it off?

Bruce Momjian <bruce@momjian.us> writes:

Yes. I bet someone set it without reading our docs:

...

We might need to clarify this sentence to be clearer it is _only_ for
testing.

On Sat, Apr 03, 2021 at 11:39:19AM -0400, Tom Lane wrote:

I wonder why it is listed under planner options at all, and not under
developer options.

On Sat, Apr 3, 2021 at 10:41:14AM -0500, Justin Pryzby wrote:

Because it's there to help DBAs catch errors in functions incorrectly marked as
parallel safe.

On Sat, Apr 03, 2021 at 11:43:36AM -0400, Bruce Momjian wrote:

Uh, isn't that developer/debugging?

I understood "developer" to mean someone who's debugging postgres itself, not
(say) a function written using pl/pgsql. Like backtrace_functions,
post_auth_delay, jit_profiling_support.

But I see that some "dev" options are more user-facing (for a sufficiently
advanced user):
ignore_checksum_failure, ignore_invalid_pages, zero_damaged_pages.

Also, I understood this to mean the "category" in pg_settings, but I guess
what's important here is the absense of the GUC in the sample/template config
file. pg_settings.category and the sample headings it appears are intended to
be synchronized, but a few of them are out of sync. See attached.

+1 to move this to "developer" options and remove it from the sample config:

# - Other Planner Options -
#force_parallel_mode = off

--
Justin

Noted thanks!!

On Sun, Apr 4, 2021 at 4:19 PM Pavel Stehule <pavel.stehule@gmail.com>
wrote:

The previous patches accidentally included some unrelated changes.

--
Justin

On Sat, Apr 03, 2021 at 08:25:46PM -0500, Justin Pryzby wrote:

Forking this thread
/messages/by-id/20210403154336.GG29125@momjian.us

Didn't see this one, thanks for forking.

I understood "developer" to mean someone who's debugging postgres itself, not
(say) a function written using pl/pgsql. Like backtrace_functions,
post_auth_delay, jit_profiling_support.

But I see that some "dev" options are more user-facing (for a sufficiently
advanced user):
ignore_checksum_failure, ignore_invalid_pages, zero_damaged_pages.

Also, I understood this to mean the "category" in pg_settings, but I guess
what's important here is the absense of the GUC in the sample/template config
file. pg_settings.category and the sample headings it appears are intended to
be synchronized, but a few of them are out of sync. See attached.

+1 to move this to "developer" options and remove it from the sample config:

# - Other Planner Options -
#force_parallel_mode = off

0001 has some changes to pg_config_manual.h related to valgrind and
memory randomization. You may want to remove that before posting a
patch.

-       {"track_commit_timestamp", PGC_POSTMASTER, REPLICATION,
+       {"track_commit_timestamp", PGC_POSTMASTER, REPLICATION_SENDING,
I can get behind this change for clarity where it gets actively used.

-       {"track_activity_query_size", PGC_POSTMASTER, RESOURCES_MEM,
+       {"track_activity_query_size", PGC_POSTMASTER, STATS_COLLECTOR,
But not this one, because it is a memory setting.

-       {"force_parallel_mode", PGC_USERSET, QUERY_TUNING_OTHER,
+       {"force_parallel_mode", PGC_USERSET, DEVELOPER_OPTIONS,
And not this one either, as it is mainly a planner thing, like the
other parameters in the same area.

The last change is related to log_autovacuum_min_duration, and I can
get behind the argument you are making to group all log activity
parameters together.  Now, about this part:
+#log_autovacuum_min_duration = -1  # -1 disables, 0 logs all actions and
+                   # their durations, > 0 logs only
+                   # actions running at least this number
+                   # of milliseconds.
I think that we should clarify in the description that this is an
autovacuum-only thing, say by appending a small sentence about the
fact that it logs autovacuum activities, in a similar fashion to
log_temp_files.  Moving the parameter out of the autovacuum section
makes it lose a bit of context.

@@ -6903,6 +6903,7 @@ fetch_more_data_begin(AsyncRequest *areq)
char sql[64];

Assert(!fsstate->conn_state->pendingAreq);
+ Assert(fsstate->conn);
What's this diff doing here?
--
Michaelx

On Fri, Apr 09, 2021 at 10:50:53AM +0900, Michael Paquier wrote:

On Sat, Apr 03, 2021 at 08:25:46PM -0500, Justin Pryzby wrote:

Forking this thread
/messages/by-id/20210403154336.GG29125@momjian.us

Didn't see this one, thanks for forking.

-       {"force_parallel_mode", PGC_USERSET, QUERY_TUNING_OTHER,
+       {"force_parallel_mode", PGC_USERSET, DEVELOPER_OPTIONS,
And not this one either, as it is mainly a planner thing, like the
other parameters in the same area.

This is the main motive behind the patch.

Developer options aren't shown in postgresql.conf.sample, which it seems like
sometimes people read through quickly, setting a whole bunch of options that
sound good, sometimes including this one. And in the best case they then ask
on -performance why their queries are slow and we tell them to turn it back off
to fix their issues. This changes to no longer put it in .sample, and calling
it a "dev" option seems to be the classification and mechanism by which to do
that.

--
Justin

ps, Maybe you saw that I'd already resent without including the accidental junk
hunks.

On Thu, Apr 8, 2021 at 10:17:18PM -0500, Justin Pryzby wrote:

On Fri, Apr 09, 2021 at 10:50:53AM +0900, Michael Paquier wrote:

On Sat, Apr 03, 2021 at 08:25:46PM -0500, Justin Pryzby wrote:

Forking this thread
/messages/by-id/20210403154336.GG29125@momjian.us

Didn't see this one, thanks for forking.

-       {"force_parallel_mode", PGC_USERSET, QUERY_TUNING_OTHER,
+       {"force_parallel_mode", PGC_USERSET, DEVELOPER_OPTIONS,
And not this one either, as it is mainly a planner thing, like the
other parameters in the same area.

This is the main motive behind the patch.

Developer options aren't shown in postgresql.conf.sample, which it seems like
sometimes people read through quickly, setting a whole bunch of options that
sound good, sometimes including this one. And in the best case they then ask
on -performance why their queries are slow and we tell them to turn it back off
to fix their issues. This changes to no longer put it in .sample, and calling
it a "dev" option seems to be the classification and mechanism by which to do
that.

+1

--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com

If only the physical world exists, free will is an illusion.

On Fri, Apr 09, 2021 at 07:39:28AM -0400, Bruce Momjian wrote:

On Thu, Apr 8, 2021 at 10:17:18PM -0500, Justin Pryzby wrote:

This is the main motive behind the patch.

Developer options aren't shown in postgresql.conf.sample, which it seems like
sometimes people read through quickly, setting a whole bunch of options that
sound good, sometimes including this one. And in the best case they then ask
on -performance why their queries are slow and we tell them to turn it back off
to fix their issues. This changes to no longer put it in .sample, and calling
it a "dev" option seems to be the classification and mechanism by which to do
that.

+1

Hm. I can see the point you are making based on the bug report that
has led to this thread:
/messages/by-id/CAN0SRDFV=Fv0zXHCGbh7gh=MTfw05Xd1x7gjJrZs5qn-TEphOw@mail.gmail.com

However, I'd like to think that we can do better than what's proposed
in the patch. There are a couple of things to consider here:
- Should the parameter be renamed to reflect that it should only be
used for testing purposes?
- Should we make more general the description of the developer options
in the docs?

I have applied the patch for log_autovacuum_min_duration for now, as
this one is clearly wrong.
--
Michael

On Mon, Apr 12, 2021 at 10:31 AM Michael Paquier <michael@paquier.xyz> wrote:

On Fri, Apr 09, 2021 at 07:39:28AM -0400, Bruce Momjian wrote:

On Thu, Apr 8, 2021 at 10:17:18PM -0500, Justin Pryzby wrote:

This is the main motive behind the patch.

Developer options aren't shown in postgresql.conf.sample, which it seems like
sometimes people read through quickly, setting a whole bunch of options that
sound good, sometimes including this one. And in the best case they then ask
on -performance why their queries are slow and we tell them to turn it back off
to fix their issues. This changes to no longer put it in .sample, and calling
it a "dev" option seems to be the classification and mechanism by which to do
that.

+1

Hm. I can see the point you are making based on the bug report that
has led to this thread:
/messages/by-id/CAN0SRDFV=Fv0zXHCGbh7gh=MTfw05Xd1x7gjJrZs5qn-TEphOw@mail.gmail.com

However, I'd like to think that we can do better than what's proposed
in the patch. There are a couple of things to consider here:
- Should the parameter be renamed to reflect that it should only be
used for testing purposes?
- Should we make more general the description of the developer options
in the docs?

IMO, categorizing force_parallel_mode to DEVELOPER_OPTIONS and moving
it to the "Developer Options" section in config.sgml looks
appropriate. So, the v2-0004 patch proposed by Justin at [1]/messages/by-id/20210408213812.GA18734@telsasoft.com looks
good to me. If there are any other GUCs that are not meant to be used
in production, IMO we could follow the same.

[1]: /messages/by-id/20210408213812.GA18734@telsasoft.com

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

Michael Paquier <michael@paquier.xyz> writes:

However, I'd like to think that we can do better than what's proposed
in the patch. There are a couple of things to consider here:
- Should the parameter be renamed to reflect that it should only be
used for testing purposes?

-1 to that part, because it would break a bunch of buildfarm animals'
configurations. I doubt that any gain in clarity would be worth it.

- Should we make more general the description of the developer options
in the docs?

Perhaps ... what did you have in mind?

regards, tom lane

On Mon, Apr 12, 2021 at 01:40:52AM -0400, Tom Lane wrote:

Michael Paquier <michael@paquier.xyz> writes:

However, I'd like to think that we can do better than what's proposed
in the patch. There are a couple of things to consider here:
- Should the parameter be renamed to reflect that it should only be
used for testing purposes?

-1 to that part, because it would break a bunch of buildfarm animals'
configurations. I doubt that any gain in clarity would be worth it.

Okay.

- Should we make more general the description of the developer options
in the docs?

Perhaps ... what did you have in mind?

The first sentence of the page now says that:
"The following parameters are intended for work on the PostgreSQL
source code, and in some cases to assist with recovery of severely
damaged databases."

That does not stick with force_parallel_mode IMO. Maybe:
"The following parameters are intended for development work related to
PostgreSQL. Some of them work on the PostgreSQL source code, some of
them can be used to control the run-time behavior of the server, and
in some cases they can be used to assist with the recovery of severely
damaged databases."
--
Michael

On Tue, Apr 13, 2021 at 04:34:23PM +0900, Michael Paquier wrote:

On Mon, Apr 12, 2021 at 01:40:52AM -0400, Tom Lane wrote:

- Should we make more general the description of the developer options
in the docs?

Perhaps ... what did you have in mind?

The first sentence of the page now says that:
"The following parameters are intended for work on the PostgreSQL
source code, and in some cases to assist with recovery of severely
damaged databases."

That does not stick with force_parallel_mode IMO. Maybe:

Good point.

--
Justin

Michael Paquier <michael@paquier.xyz> writes:

On Mon, Apr 12, 2021 at 01:40:52AM -0400, Tom Lane wrote:

Perhaps ... what did you have in mind?

The first sentence of the page now says that:
"The following parameters are intended for work on the PostgreSQL
source code, and in some cases to assist with recovery of severely
damaged databases."

That does not stick with force_parallel_mode IMO. Maybe:
"The following parameters are intended for development work related to
PostgreSQL. Some of them work on the PostgreSQL source code, some of
them can be used to control the run-time behavior of the server, and
in some cases they can be used to assist with the recovery of severely
damaged databases."

I think that's overly wordy. Maybe

The following parameters are intended for developer testing, and
should never be enabled for production work. However, some of
them can be used to assist with the recovery of severely
damaged databases.

regards, tom lane

On Tue, Apr 13, 2021 at 10:12:35AM -0400, Tom Lane wrote:

The following parameters are intended for developer testing, and
should never be enabled for production work. However, some of
them can be used to assist with the recovery of severely
damaged databases.

Okay, that's fine by me.
--
Michael

On Tue, Apr 13, 2021 at 07:31:39AM -0500, Justin Pryzby wrote:

Good point.

Thanks. I have used the wording that Tom has proposed upthread, added
one GUC_NOT_IN_SAMPLE that you forgot, and applied the
force_parallel_mode patch.
--
Michael

On Wed, Apr 14, 2021 at 03:57:21PM +0900, Michael Paquier wrote:

On Tue, Apr 13, 2021 at 07:31:39AM -0500, Justin Pryzby wrote:

Good point.

Thanks. I have used the wording that Tom has proposed upthread, added
one GUC_NOT_IN_SAMPLE that you forgot, and applied the
force_parallel_mode patch.

Thanks. It just occured to me to ask if we should backpatch it.
The goal is to avoid someone trying to use this as a peformance option.

It's to their benefit and ours if they don't do that on v10-13 for the next 5
years, not just v14-17.

The patch seems to apply cleanly on v12 but cherry-pick needs help for other
branches...

--
Justin

On Fri, Apr 23, 2021 at 01:23:26PM -0500, Justin Pryzby wrote:

The patch seems to apply cleanly on v12 but cherry-pick needs help for other
branches...

FWIW, this did not seem bad enough to me to require a back-patch.
This parameter got introduced in 2016 and this was the only report
related to it for the last 5 years.
--
Michael

On Sat, Apr 24, 2021 at 10:50:21AM +0900, Michael Paquier wrote:

On Fri, Apr 23, 2021 at 01:23:26PM -0500, Justin Pryzby wrote:

The patch seems to apply cleanly on v12 but cherry-pick needs help for other
branches...

FWIW, this did not seem bad enough to me to require a back-patch.
This parameter got introduced in 2016 and this was the only report
related to it for the last 5 years.

No, it's not the first report - although I'm surprised I wasn't able to find
more than these.

/messages/by-id/20190102164525.GU25379@telsasoft.com
/messages/by-id/CAKJS1f_Qi0iboCos3wu6QiAbdF-9FoK57wxzKbe2-WcesN4rFA@mail.gmail.com

--
Justin

On Fri, Apr 09, 2021 at 10:50:53AM +0900, Michael Paquier wrote:

-       {"track_commit_timestamp", PGC_POSTMASTER, REPLICATION,
+       {"track_commit_timestamp", PGC_POSTMASTER, REPLICATION_SENDING,
I can get behind this change for clarity where it gets actively used.

I'm not sure what you meant?

...but, I realized just now that *zero* other GUCs use "REPLICATION".
And the documentation puts it in 20.6.1. Sending Servers,
so it still seems to me that this is correct to move this, too.

https://www.postgresql.org/docs/devel/runtime-config-replication.html

Then, I wonder if REPLICATION should be removed from guc_tables.h...

--
Justin

Justin Pryzby <pryzby@telsasoft.com> writes:

...but, I realized just now that *zero* other GUCs use "REPLICATION".
And the documentation puts it in 20.6.1. Sending Servers,
so it still seems to me that this is correct to move this, too.
https://www.postgresql.org/docs/devel/runtime-config-replication.html
Then, I wonder if REPLICATION should be removed from guc_tables.h...

For the archives' sake --- these things are now committed as part of
a55a98477. I'd forgotten this thread, and then rediscovered the same
inconsistencies as Justin had while reviewing Bharath Rupireddy's patch
for bug #16997 [1]/messages/by-id/16997-ff16127f6e0d1390@postgresql.org.

I think this thread can now be closed off as done. However, there
are some open issues mentioned in the other thread, if anyone here
wants to comment.

regards, tom lane

[1]: /messages/by-id/16997-ff16127f6e0d1390@postgresql.org