Re: There should be a way to use the force flag when restoring databases

[Ahmed Ibrahim]

Hi all,

I have addressed the pg version compatibility with the FORCE option in
drop. Here is the last version of the patch

On Tue, Aug 1, 2023 at 6:19 PM Ahmed Ibrahim 
wrote:

> Hi Gurjeet,
>
> I have addressed all your comments except for the tests.
>
> I have tried adding test cases but I wasn't able to do it as it's in my
> mind. I am not able to do things like having connections to the database
> and trying to force the restore, then it will complete successfully
> otherwise it shows errors.
>
> In the meantime I will continue trying to do the test cases. If anyone can
> help on that, I will appreciate it.
>
> Thanks
>
> On Thu, Jul 27, 2023 at 1:36 AM Gurjeet Singh  wrote:
>
>> On Sun, Jul 23, 2023 at 6:09 AM Ahmed Ibrahim
>>  wrote:
>> >
>> > Hi everyone,
>> >
>> > I have been working on this. This is a proposed patch for it so we have
>> a force option for DROPping the database.
>> >
>> > I'd appreciate it if anyone can review.
>>
>> Hi Ahmed,
>>
>> Thanks for working on this patch!
>>
>> +
>> +int force;
>>
>> That extra blank line is unnecessary.
>>
>> Using the bool data type, instead of int, for this option would've
>> more natural.
>>
>> +if (ropt->force){
>>
>> Postgres coding style is to place the curly braces on a new line,
>> by themselves.
>>
>> +char   *dropStmt = pg_strdup(te->dropStmt);
>>
>> See if you can use pnstrdup(). Using that may obviate the need for
>> doing the null-placement acrobatics below.
>>
>> +PQExpBuffer ftStmt = createPQExpBuffer();
>>
>> What does the 'ft' stand for in this variable's name?
>>
>> +dropStmt[strlen(dropStmt) - 2] = ' ';
>> +dropStmt[strlen(dropStmt) - 1] = '\0';
>>
>> Try to evaluate the strlen() once and reuse it.
>>
>> +appendPQExpBufferStr(ftStmt, dropStmt);
>> +appendPQExpBufferStr(ftStmt, "WITH(FORCE);");
>> +te->dropStmt = ftStmt->data;
>> +}
>> +
>>
>> Remove the extra trailing whitespace on that last blank line.
>>
>> I think this whole code block needs to be protected by an 'if
>> (ropt->createDB)' check, like it's done about 20 lines above this
>> hunk. Otherwise, we may be appending 'WITH (FORCE)' for the DROP
>> command of a different (not a database) object type.
>>
>> Also, you may want to check that the target database version is
>> one that supports WITH force option. This command will fail for
>> anything before v13.
>>
>> The patch needs doc changes (pg_restore.sgml). And it needs to
>> mention --force option in the help output, as well (usage() function).
>>
>> Can you please see if you can add appropriate test case for this.
>> The committers may insist on it, when reviewing.
>>
>> Here are a couple of helpful links on how to prepare and submit
>> patches to the community. You may not need to strictly adhere to
>> these, but try to pick up a few recommendations that would make the
>> reviewer's job a bit easier.
>>
>> [1]: https://wiki.postgresql.org/wiki/Creating_Clean_Patches
>> [2]: https://wiki.postgresql.org/wiki/Submitting_a_Patch
>>
>> Best regards,
>> Gurjeet
>> http://Gurje.et
>>
>
diff --git a/doc/src/sgml/ref/pg_restore.sgml b/doc/src/sgml/ref/pg_restore.sgml
index 47bd7dbda0..f24e06fdf7 100644
--- a/doc/src/sgml/ref/pg_restore.sgml
+++ b/doc/src/sgml/ref/pg_restore.sgml
@@ -726,6 +726,15 @@ PostgreSQL documentation

  
 
+ 
+  --force
+  
+   
+ Force database to drop while restoring if there are any connections.
+   
+  
+ 
+
 

 
diff --git a/src/bin/pg_dump/pg_backup.h b/src/bin/pg_dump/pg_backup.h
index aba780ef4b..2d167b58bf 100644
--- a/src/bin/pg_dump/pg_backup.h
+++ b/src/bin/pg_dump/pg_backup.h
@@ -154,6 +154,7 @@ typedef struct _restoreOptions
 	int			enable_row_security;
 	int			sequence_data;	/* dump sequence data even in schema-only mode */
 	int			binary_upgrade;
+	bool		force;
 } RestoreOptions;
 
 typedef struct _dumpOptions
diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
index 39ebcfec32..218d440e35 100644
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -532,6 +532,15 @@ Rest

Re: There should be a way to use the force flag when restoring databases

[Ahmed Ibrahim]

Hi Gurjeet,

I have addressed all your comments except for the tests.

I have tried adding test cases but I wasn't able to do it as it's in my
mind. I am not able to do things like having connections to the database
and trying to force the restore, then it will complete successfully
otherwise it shows errors.

In the meantime I will continue trying to do the test cases. If anyone can
help on that, I will appreciate it.

Thanks

On Thu, Jul 27, 2023 at 1:36 AM Gurjeet Singh  wrote:

> On Sun, Jul 23, 2023 at 6:09 AM Ahmed Ibrahim
>  wrote:
> >
> > Hi everyone,
> >
> > I have been working on this. This is a proposed patch for it so we have
> a force option for DROPping the database.
> >
> > I'd appreciate it if anyone can review.
>
> Hi Ahmed,
>
> Thanks for working on this patch!
>
> +
> +int force;
>
> That extra blank line is unnecessary.
>
> Using the bool data type, instead of int, for this option would've
> more natural.
>
> +if (ropt->force){
>
> Postgres coding style is to place the curly braces on a new line,
> by themselves.
>
> +char   *dropStmt = pg_strdup(te->dropStmt);
>
> See if you can use pnstrdup(). Using that may obviate the need for
> doing the null-placement acrobatics below.
>
> +PQExpBuffer ftStmt = createPQExpBuffer();
>
> What does the 'ft' stand for in this variable's name?
>
> +dropStmt[strlen(dropStmt) - 2] = ' ';
> +dropStmt[strlen(dropStmt) - 1] = '\0';
>
> Try to evaluate the strlen() once and reuse it.
>
> +appendPQExpBufferStr(ftStmt, dropStmt);
> +appendPQExpBufferStr(ftStmt, "WITH(FORCE);");
> +te->dropStmt = ftStmt->data;
> +}
> +
>
> Remove the extra trailing whitespace on that last blank line.
>
> I think this whole code block needs to be protected by an 'if
> (ropt->createDB)' check, like it's done about 20 lines above this
> hunk. Otherwise, we may be appending 'WITH (FORCE)' for the DROP
> command of a different (not a database) object type.
>
> Also, you may want to check that the target database version is
> one that supports WITH force option. This command will fail for
> anything before v13.
>
> The patch needs doc changes (pg_restore.sgml). And it needs to
> mention --force option in the help output, as well (usage() function).
>
> Can you please see if you can add appropriate test case for this.
> The committers may insist on it, when reviewing.
>
> Here are a couple of helpful links on how to prepare and submit
> patches to the community. You may not need to strictly adhere to
> these, but try to pick up a few recommendations that would make the
> reviewer's job a bit easier.
>
> [1]: https://wiki.postgresql.org/wiki/Creating_Clean_Patches
> [2]: https://wiki.postgresql.org/wiki/Submitting_a_Patch
>
> Best regards,
> Gurjeet
> http://Gurje.et
>
diff --git a/doc/src/sgml/ref/pg_restore.sgml b/doc/src/sgml/ref/pg_restore.sgml
index 47bd7dbda0..f24e06fdf7 100644
--- a/doc/src/sgml/ref/pg_restore.sgml
+++ b/doc/src/sgml/ref/pg_restore.sgml
@@ -726,6 +726,15 @@ PostgreSQL documentation

  
 
+ 
+  --force
+  
+   
+ Force database to drop while restoring if there are any connections.
+   
+  
+ 
+
 

 
diff --git a/src/bin/pg_dump/pg_backup.h b/src/bin/pg_dump/pg_backup.h
index aba780ef4b..2d167b58bf 100644
--- a/src/bin/pg_dump/pg_backup.h
+++ b/src/bin/pg_dump/pg_backup.h
@@ -154,6 +154,7 @@ typedef struct _restoreOptions
 	int			enable_row_security;
 	int			sequence_data;	/* dump sequence data even in schema-only mode */
 	int			binary_upgrade;
+	bool		force;
 } RestoreOptions;
 
 typedef struct _dumpOptions
diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
index 39ebcfec32..218d440e35 100644
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -532,6 +532,15 @@ RestoreArchive(Archive *AHX)
  */
 if (*te->dropStmt != '\0')
 {
+	if (ropt->createDB && ropt->force)
+	{
+		int queryLen = strlen(te->dropStmt);
+		char	   *dropStmt = pnstrdup(te->dropStmt, queryLen - 2);
+		PQExpBuffer newStmt = createPQExpBuffer();
+		appendPQExpBufferStr(newStmt, dropStmt);
+		appendPQExpBufferStr(newStmt, " WITH(FORCE);");
+		te->dropStmt = newStmt->data;
+	}
 	if (!ropt->if_exists ||
 		strncmp(te->dropStmt, "--", 

Re: There should be a way to use the force flag when restoring databases

[Ahmed Ibrahim]

Hi everyone,

I have been working on this. This is a proposed patch for it so we have a
force option for DROPping the database.

I'd appreciate it if anyone can review.



On Thu, Jul 20, 2023 at 9:36 PM Gurjeet Singh  wrote:

> On Thu, Jul 20, 2023 at 2:10 AM Daniel Gustafsson  wrote:
> >
> > > On 19 Jul 2023, at 19:28, Gurjeet Singh  wrote:
> > >
> > > The docs for 'pg_restore --create` say "Create the database before
> > > restoring into it. If --clean is also specified, drop and recreate the
> > > target database before connecting to it."
> > >
> > > If we provided a force option, it may then additionally say: "If the
> > > --clean and --force options are specified, DROP DATABASE ... WITH
> > > FORCE command will be used to drop the database."
> >
> > pg_restore --clean refers to dropping any pre-existing database objects
> and not
> > just databases, but --force would only apply to databases.
> >
> > I wonder if it's worth complicating pg_restore with that when running
> dropdb
> > --force before pg_restore is an option for those wanting to use WITH
> FORCE.
>
> Fair point. But the same argument could've been applied to --clean
> option, as well; why overload the meaning of --clean and make it drop
> database, when a dropdb before pg_restore was an option.
>
> IMHO, if pg_restore offers to drop database, providing an option to
> the user to do it forcibly is not that much of a stretch, and within
> reason for the user to expect it to be there, like Joan did.
>
> Best regards,
> Gurjeet
> http://Gurje.et
>
>
>
diff --git a/src/bin/pg_dump/pg_backup.h b/src/bin/pg_dump/pg_backup.h
index aba780ef4b..aeec3c8dcb 100644
--- a/src/bin/pg_dump/pg_backup.h
+++ b/src/bin/pg_dump/pg_backup.h
@@ -154,6 +154,8 @@ typedef struct _restoreOptions
 	int			enable_row_security;
 	int			sequence_data;	/* dump sequence data even in schema-only mode */
 	int			binary_upgrade;
+
+	int			force;
 } RestoreOptions;
 
 typedef struct _dumpOptions
diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
index 39ebcfec32..cc13be841a 100644
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -532,6 +532,16 @@ RestoreArchive(Archive *AHX)
  */
 if (*te->dropStmt != '\0')
 {
+	if (ropt->force){
+		char	   *dropStmt = pg_strdup(te->dropStmt);
+		PQExpBuffer ftStmt = createPQExpBuffer();
+		dropStmt[strlen(dropStmt) - 2] = ' ';
+		dropStmt[strlen(dropStmt) - 1] = '\0';
+		appendPQExpBufferStr(ftStmt, dropStmt);
+		appendPQExpBufferStr(ftStmt, "WITH(FORCE);");
+		te->dropStmt = ftStmt->data;
+	}
+	
 	if (!ropt->if_exists ||
 		strncmp(te->dropStmt, "--", 2) == 0)
 	{
diff --git a/src/bin/pg_dump/pg_restore.c b/src/bin/pg_dump/pg_restore.c
index 049a100634..02347e86fb 100644
--- a/src/bin/pg_dump/pg_restore.c
+++ b/src/bin/pg_dump/pg_restore.c
@@ -74,6 +74,7 @@ main(int argc, char **argv)
 	static int	no_security_labels = 0;
 	static int	no_subscriptions = 0;
 	static int	strict_names = 0;
+	static int	force = 0;
 
 	struct option cmdopts[] = {
 		{"clean", 0, NULL, 'c'},
@@ -123,6 +124,7 @@ main(int argc, char **argv)
 		{"no-publications", no_argument, &no_publications, 1},
 		{"no-security-labels", no_argument, &no_security_labels, 1},
 		{"no-subscriptions", no_argument, &no_subscriptions, 1},
+		{"force", no_argument, &force, 1},
 
 		{NULL, 0, NULL, 0}
 	};
@@ -351,6 +353,7 @@ main(int argc, char **argv)
 	opts->no_publications = no_publications;
 	opts->no_security_labels = no_security_labels;
 	opts->no_subscriptions = no_subscriptions;
+	opts->force = force;
 
 	if (if_exists && !opts->dropSchema)
 		pg_fatal("option --if-exists requires option -c/--clean");

Issue in _bt_getrootheight

[Ahmed Ibrahim]

Hi everyone,

We have been working on the pg_adviser
 extension whose goal is to
suggest indexes by creating virtual/hypothetical indexes and see how it
affects the query cost.

The hypothetical index shouldn't take any space on the disk (allocates 0
pages) so we give it the flag *INDEX_CREATE_SKIP_BUILD.*
But the problem comes from here when the function *get_relation_info *is
called in planning stage, it tries to calculate the B-Tree height by
calling function *_bt_getrootheight*, but the B-Tree is not built at all,
and its metadata page (which is block 0 in our case) doesn't exist, so this
returns error that it cannot read the page (since it doesn't exist).

I tried to debug the code and found that this feature was introduced in
version 9.3 under this commit [1]. I think that in the code we need to
check if it's a B-Tree index *AND *the index is built/have some pages, then
we can go and calculate it otherwise just put it to -1

I mean instead of this
if (info->relam == BTREE_AM_OID)
{
/* For btrees, get tree height while we have the index open */
info->tree_height = _bt_getrootheight(indexRelation);
}
else
{
/* For other index types, just set it to "unknown" for now */
info->tree_height = -1;
}

The first line should be
if (info->relam == BTREE_AM_OID && info->pages > 0)
or use the storage manager (smgr) to know if the first block exists.

I would appreciate it if anyone can agree/approve or deny so that I know if
anything I am missing :)

Thanks everyone :)

[1]
https://github.com/postgres/postgres/commit/31f38f28b00cbe2b9267205359e3cf7bafa1cb97

Re: Inquiry/Help with pg_adviser (problem in index_create function for creating indexes)

[Ahmed Ibrahim]

Hi,

Since some people prefer plain text over screenshots/pdfs (but I think the
pdf is more readable), I will post the problem here, in case anyone can
help. I will appreciate that :)

The full current code (PR is still draft) can be found at
https://github.com/DrPostgres/pg_adviser/pull/4

The idea behind what is being done is creating virtual indexes, and
measuring the query cost after creating those indexes, and see whether we
will get a better cost or not, and maximize the benefit from those choices.
So far, the project is okay and compiling/working successfully (with
Postgres 16), but the problem is when creating
the virtual indexes (with version 16), I give it flag
*INDEX_CREATE_SKIP_BUILD* (just like it was with version 8.3 and was
working)

After that, the index gets created successfully, but when trying to call
*standard_planner* for the same query with the new index created (to see
how the query cost changed), I get the following error
==
2023-06-24 19:09:21.843 EEST [45000] ERROR: could not read block 0 in file
"base/16384/139323": read only 0 of 8192 bytes
2023-06-24 19:09:21.843 EEST [45000] STATEMENT: explain select * from t
where a > 5000;
ERROR: could not read block 0 in file "base/16384/139323": read only 0 of
8192 bytes
=

I tried too many things, like letting it build the whole index, or
*REINDEX *ing it after being created. I also debugged
PostgreSQL source code to see where it stops, but wasn’t able to solve the
problem.
When trying to let it build the Index, the function *index_build* gets
errors

One last thing I tried is giving it flag *INDEX_CREATE_SKIP_BUILD* and
*INDEX_CREATE_CONCURRENT
*, the index gets created
successfully but when doing so, the query cost never changes, and the query
never uses the index. When I try to
*REINDEX* it, I just get that query is aborted.

Although I think it might be a trivial thing I might have forgotten :D, I
would appreciate any help as I have been
trying to fix this for more than 2 days.

Some screenshots can be found in the pdf mentioned in the first mail.

Thanks all

On Sun, Jun 25, 2023 at 2:50 AM Ahmed Ibrahim 
wrote:

> Hi everyone!
>
> I am new to PostgreSQL community and working currently on project
> pg_adviser [https://github.com/DrPostgres/pg_adviser/]
>
> The extension last worked with version 8.3, and currently I am working to
> make it support version 16 and then the other active versions.
>
> I will give a brief about the extension:
> It's used to recommend useful indexes for a set of queries. It does that
> by  planning the query initially and seeing the initial cost and then
> creating *virtual* indexes (based on the query and columns used in it,
> ..etc) and planning again to see how those indexes changed the cost.
>
> The problem I am facing is in creating those indexes in Postgres 16 (while
> calling *index_create*), and you can find here a detail description about
> the problem along with the code/PR
> https://drive.google.com/file/d/1x2PnDEfEo094vgNiBd1-BfJtB5Fovrih/view
>
> I would appreciate any help. Thanks :)
>
>

Inquiry/Help with pg_adviser (problem in index_create function for creating indexes)

[Ahmed Ibrahim]

Hi everyone!

I am new to PostgreSQL community and working currently on project
pg_adviser [https://github.com/DrPostgres/pg_adviser/]

The extension last worked with version 8.3, and currently I am working to
make it support version 16 and then the other active versions.

I will give a brief about the extension:
It's used to recommend useful indexes for a set of queries. It does that
by  planning the query initially and seeing the initial cost and then
creating *virtual* indexes (based on the query and columns used in it,
..etc) and planning again to see how those indexes changed the cost.

The problem I am facing is in creating those indexes in Postgres 16 (while
calling *index_create*), and you can find here a detail description about
the problem along with the code/PR
https://drive.google.com/file/d/1x2PnDEfEo094vgNiBd1-BfJtB5Fovrih/view

I would appreciate any help. Thanks :)