Tidy up log fdw #6

BRupireddy2 · 2022-12-19T09:47:01Z

Issue #, if available: NA

Description of changes:

Tidy up log_fdw

This commit basically cleans up the log_fdw extension with the following
notable changes:

Wraps list_postgres_log_files() with pg_ls_logdir() to not break
compatibility.
Allows pg_read_server_files roles to execute list_postgres_log_files()
and create_foreign_table_for_log_file(). Additionally, allows
pg_monitor role to execute list_postgres_log_files() similar to
pg_ls_logdir().
Adds tests to cover the extension code.
Run pgindent on log_fdw.c

Note that the log_fdw extension is verified on master, REL_15_STABLE and REL_14_STABLE. In the PG versions REL_13_STABLE and prior, the COPY...FROM APIs were a bit different hence the log_fdw extension don't get compiled. To make the log_fdw compatible until all supported PG version (REL_11_STABLE), we might have to use version macro PG_VERSION_NUM to differentiate the code.

PS: It will be good to add the log_fdw tests to CI. However, the CI pipeline isn't able to run 'make check', it fails with "make check" is not supported. The CI pipeline currently runs 'make installcheck'. Note that when a test uses custom conf file for specific GUC settings, 'make installcheck' isn't supported for such tests. And log_fdw tests use custom conf file for enabling syslogger and other GUC settings because of which 'make installcheck' had to be disabled.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

This commit basically cleans up the log_fdw extension with the following notable changes: 1. Wraps list_postgres_log_files() with pg_ls_logdir() to not break compatibility. 2. Allows pg_read_server_files roles to execute list_postgres_log_files() and create_foreign_table_for_log_file(). Additionally, allows pg_monitor role to execute list_postgres_log_files() similar to pg_ls_logdir(). 3. Adds tests to cover the extension code.

sharmay

Some comments and questions
Todo: review .c

sharmay · 2022-12-19T10:47:38Z

Makefile


+ifdef USE_PGXS


I heard from @jkatz that USE_PGXS is default going forward and based on this it was removed in pg_tle and here.

May be. However, the Makefile is inline with all other core and standard extensions out there. Even when that change comes in, we might retain it this way for backward compatibility reasons. And log_fdw is expected to be used with PG 14 and PG 15 too, so we can have this standard way.

aws/pg_tle@d0f90c3

There's a problem with that approach. Say, I'm a developer and I typically copy any external extension to my Postgres source code's src/contrib module and then try to do 'make install' and from there I run tests with 'make check' - without the changes in Makefile as done in PR, it doesn't allow me to build the extension on my developer branch.

So, doing so as in the log_fdw's master or pg_tle or elsewhere is a developer unfriendly IMO.

We have to define "developer" in this context. Is it someone working on PostgreSQL itself or an extension?

For extension development, I have not had this issue. I just ensure the binary copy of pg_config is in my $PATH

@jkatz - note that without else block [1] (including Makefile.global and contrib-global.mk) one can't run the tests using "make check", although with just setting pg_config path in $PATH, one might be able to compile the extension code with "make install".

In short, we need this to be able to run tests.

For instance, we can enable CI pipeline to run log_fdw extension's tests - https://github.com/rupireddAWS/postgresql-logfdw/tree/tidy_up_and_add_tests_to_CI_pipeline. See the newly added tests getting executed here - https://github.com/rupireddAWS/postgresql-logfdw/actions/runs/3739842274/jobs/6347524992.

[1]

ifdef USE_PGXS PG_CONFIG = pg_config PGXS := $(shell $(PG_CONFIG) --pgxs) include $(PGXS) else subdir = contrib/postgresql-logfdw top_builddir = ../.. include $(top_builddir)/src/Makefile.global include $(top_srcdir)/contrib/contrib-global.mk endif

OK. We may need to adjust pg_tle then too.

sharmay · 2022-12-19T11:04:20Z

log_fdw--1.4.sql

+ * core function pg_ls_logdir().
+ */
+GRANT EXECUTE ON FUNCTION list_postgres_log_files() TO pg_read_server_files, pg_monitor;
+GRANT EXECUTE ON FUNCTION create_foreign_table_for_log_file(text, text, text) TO pg_read_server_files;


Reason for not granting this execute to pg_monitor?

list_postgres_log_files() just looks at the files, not the contents of the file and is a wrapper around pg_ls_logdir() which is also allowed by pg_monitor.
Whereas create_foreign_table_for_log_file() allows one to look at the contents of the server log files hence I thought pg_read_server_files role best fits it not the pg_monitor.
All that said, if others agree, we can enable pg_monitor roles to execute create_foreign_table_for_log_file().

What benefit do we get by just having granting for list_postgres_log_files? List is not much of use for this role unless allowed to view the logs using fdw.

pg_ls_logdir() is allowed by pg_monitor, which means, if one wants to execute list_postgres_log_files(), it needs to be allowed by pg_monitor too.

There are two ways to deal this:

For both of log_fdw extension functions, let pg_monitor roles execute them and remove pg_read_server_files role from the picture.

Get rid of list_postgres_log_files() completely and just have the remaining log_fdw function executed by pg_read_server_files. However, this might create some compatibility issues as some might be using list_postgres_log_files() already.

All said, I prefer to do (1), as I feel that looking at/reading log files is a way of monitoring.

Thoughts?

I don't know if (1) works for environments that disallow pg_read_server_files.

I agree that granting execute permission to pg_read_server_files for extension functions has some risks. I fully understand the concern here. I'm okay with just revoking execute on all of the extension's functions out of the box. And the users of the extension can then decide to grant execute as needed. Thoughts?

Addressed this as part of the commit - 4ff524a.

sharmay · 2022-12-19T11:05:22Z

log_fdw.conf

+log_destination = 'stderr, csvlog'
+# these ensure stability of test results:
+log_rotation_age = 0
+lc_messages = 'C'


Test for other lc types?

Since, we aren't looking at the file contents, do we need with other lc_messages types? I actually pulled these settings from https://github.com/postgres/postgres/blob/master/src/bin/pg_ctl/t/004_logrotate.pl where it looks at the file contents.

Whereas the log_fdw tests don't look at the file contents, hence we may not need to test with different lc_messages.

sharmay · 2022-12-19T11:06:35Z

log_fdw.control

@@ -1,5 +1,5 @@
 # log_fdw extension
 comment = 'foreign-data wrapper for Postgres log file access'
-default_version = 'EXTVERSION'
+default_version = '1.4'


Why not create control file dynamically?

Does it have any benefit? With default version defined either dynamically or statically, one needs to compile the whole extension anyway. Static definition reduces the code a bit and is inline with all other core extensions.

as of now this is not a core extension, so far a 3rd party extension.
This makefile was mocked based on pg_tle.

It's a developer choice and embedding EXTVERSION adds an extra code - to convert .in file to .control file, define the macro, ignore those generated files in .gitignore and so on. I don't think we need to go EXTVERSION path unless there's a clear benefit.

davecramer · 2022-12-19T12:55:01Z

I find it very difficult to separate the pg_indent changes from anything else. My preference would be to have that as a separate PR after all the non-style changes go in.

This reverts commit 7685a65.

davecramer · 2022-12-19T16:38:00Z

looks good to me. Thanks!

This commit just revokes all permissions from the public out-of-the-box for the extenson's functions and not grant any permissions to anybody. This way, the users of this extension have the choice to decide whom to give permissions to. This is better from the usability perspective. Following are the minimal things that one needs to do for enabling others to use the extension's functions: CREATE ROLE foo; -- a non-superuser GRANT pg_monitor TO foo; -- needed only when list_postgres_log_files() is used GRANT CREATE ON SCHEMA bar TO foo; -- needed to create foreign tables in schema named bar GRANT USAGE ON FOREIGN SERVER log_fdw_server TO foo; SET ROLE foo; SELECT * FROM create_foreign_table_for_log_file('log_file_tbl', 'log_fdw_server', 'log_file.csv');

BRupireddy2 added 2 commits December 19, 2022 09:08

Run pgindent on log_fdw.c

7685a65

sharmay reviewed Dec 19, 2022

View reviewed changes

Revert "Run pgindent on log_fdw.c"

8df7b2e

This reverts commit 7685a65.

BRupireddy2 added 2 commits December 21, 2022 08:16

Tidy up documentation

e9806cc

davecramer merged commit 2b18797 into aws:main Dec 21, 2022


		ifdef USE_PGXS

Tidy up log fdw #6

Tidy up log fdw #6

Uh oh!

Conversation

BRupireddy2 commented Dec 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sharmay left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BRupireddy2 Dec 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BRupireddy2 Dec 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BRupireddy2 Dec 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davecramer commented Dec 19, 2022

Uh oh!

davecramer commented Dec 19, 2022

Uh oh!

Uh oh!

BRupireddy2 commented Dec 19, 2022 •

edited

Loading

BRupireddy2 Dec 19, 2022 •

edited

Loading

BRupireddy2 Dec 20, 2022 •

edited

Loading

BRupireddy2 Dec 19, 2022 •

edited

Loading