date:20190228

Re: pg_partition_tree crashes for a non-defined relation

2019-02-28 Thread Michael Paquier

On Thu, Feb 28, 2019 at 11:50:16PM -0500, Tom Lane wrote:
> FWIW, I don't agree with Michael's suggestion above.  A plain table is
> significantly different from a partitioned table with no children:
> you can store rows in the former but not the latter, and you can add
> partitions to the latter but not the former.  So conflating the two
> doesn't seem likely to lead to any good outcome.

Okay, so the opinion moves into this direction.  What would be needed
is just something like the attached patch then based on Amit's
suggestion?  The routine names and comments looked fine to me so I
have not touched the surroundings, and comments are welcome.

> But, having said that, we've learned that it's generally bad for
> catalog-query functions to fail outright just because they're pointed
> at the wrong kind of catalog object.  So I think that what we want here
> is for pg_partition_tree to return NULL or an empty set or some such
> for a plain table, while its output for a childless partitioned table
> should be visibly different from that.  I'm not wedded to details
> beyond that idea.

Yep, that's the intention since cc53123.  I won't come back to return
an ERROR in any case.  Here is what the patch gives for childless
partitions FWIW:
=# CREATE TABLE ptif_test (a int, b int) PARTITION BY range (a);
CREATE TABLE
=# SELECT * FROM pg_partition_tree('ptif_test');
   relid   | parentrelid | isleaf | level
---+-++---
 ptif_test | null| f  | 0
(1 row)
--
Michael
diff --git a/src/backend/utils/adt/partitionfuncs.c b/src/backend/utils/adt/partitionfuncs.c
index 36d9f69cbc..a2fe4f34b6 100644
--- a/src/backend/utils/adt/partitionfuncs.c
+++ b/src/backend/utils/adt/partitionfuncs.c
@@ -35,17 +35,17 @@ static bool
 check_rel_can_be_partition(Oid relid)
 {
 	char		relkind;
+	bool		relispartition;
 
 	/* Check if relation exists */
 	if (!SearchSysCacheExists1(RELOID, ObjectIdGetDatum(relid)))
 		return false;
 
 	relkind = get_rel_relkind(relid);
+	relispartition = get_rel_relispartition(relid);
 
 	/* Only allow relation types that can appear in partition trees. */
-	if (relkind != RELKIND_RELATION &&
-		relkind != RELKIND_FOREIGN_TABLE &&
-		relkind != RELKIND_INDEX &&
+	if (!relispartition &&
 		relkind != RELKIND_PARTITIONED_TABLE &&
 		relkind != RELKIND_PARTITIONED_INDEX)
 		return false;
@@ -189,13 +189,6 @@ pg_partition_root(PG_FUNCTION_ARGS)
 	if (!check_rel_can_be_partition(relid))
 		PG_RETURN_NULL();
 
-	/*
-	 * If the relation is not a partition (it may be the partition parent),
-	 * return itself as a result.
-	 */
-	if (!get_rel_relispartition(relid))
-		PG_RETURN_OID(relid);
-
 	/* Fetch the top-most parent */
 	ancestors = get_partition_ancestors(relid);
 	rootrelid = llast_oid(ancestors);
diff --git a/src/test/regress/expected/partition_info.out b/src/test/regress/expected/partition_info.out
index 73269ffd09..24e3fe60f3 100644
--- a/src/test/regress/expected/partition_info.out
+++ b/src/test/regress/expected/partition_info.out
@@ -138,19 +138,18 @@ SELECT relid, parentrelid, level, isleaf
 (6 rows)
 
 DROP TABLE ptif_test;
--- Table that is not part of any partition tree is the only member listed.
+-- Table that is not part of any partition tree is not listed.
 CREATE TABLE ptif_normal_table(a int);
 SELECT relid, parentrelid, level, isleaf
   FROM pg_partition_tree('ptif_normal_table');
-   relid   | parentrelid | level | isleaf 
+-+---+
- ptif_normal_table | | 0 | t
-(1 row)
+ relid | parentrelid | level | isleaf 
+---+-+---+
+(0 rows)
 
 SELECT pg_partition_root('ptif_normal_table');
  pg_partition_root 
 ---
- ptif_normal_table
+ 
 (1 row)
 
 DROP TABLE ptif_normal_table;
diff --git a/src/test/regress/sql/partition_info.sql b/src/test/regress/sql/partition_info.sql
index 119b90afe4..d9dfa5d5d7 100644
--- a/src/test/regress/sql/partition_info.sql
+++ b/src/test/regress/sql/partition_info.sql
@@ -64,7 +64,7 @@ SELECT relid, parentrelid, level, isleaf
 
 DROP TABLE ptif_test;
 
--- Table that is not part of any partition tree is the only member listed.
+-- Table that is not part of any partition tree is not listed.
 CREATE TABLE ptif_normal_table(a int);
 SELECT relid, parentrelid, level, isleaf
   FROM pg_partition_tree('ptif_normal_table');


signature.asc
Description: PGP signature

Re: readdir is incorrectly implemented at Windows

2019-02-28 Thread Michael Paquier

On Fri, Mar 01, 2019 at 10:23:02AM +0300, Konstantin Knizhnik wrote:
> Yes, Yuri Kurenkov and Grigory Smalking did a lot in investigation
> of this problem.
> (the irony is that the problem detected by Yuri was caused by
> another bug in pg_probackup, but we thought that it was related with
> permissions and come to this issue).

Thanks for confirming, Konstantin.  Let's wait a couple of days to see
if anybody has objections or comments, and I'll try to commit this
patch.
--
Michael


signature.asc
Description: PGP signature

Re: readdir is incorrectly implemented at Windows

2019-02-28 Thread Konstantin Knizhnik

On 01.03.2019 9:13, Michael Paquier wrote:

On Thu, Feb 28, 2019 at 11:15:53AM +0900, Michael Paquier wrote:

Could you add it to the next commit fest as a bug fix please? I think
that I will be able to look at that in details soon, but if not it
would be better to not lose track of your fix.

Okay, I have looked at your patch. And double-checked on Windows. To
put it short, I agree with the approach you are taking. I have been
curious about the mention to MinGW in the existing code as well as in
the patch you are proposing, and I have checked if what your patch and
what the current state are correct, and I think that HEAD is wrong on
one thing.

First mingw64 code can be found here:
https://sourceforge.net/p/mingw-w64/mingw-w64/ci/master/tree/
https://github.com/Alexpux/mingw-w64/

Then, the implementation of readdir/opendir/closedir can be found in
mingw-w64-crt/misc/dirent.c. Looking at their implementation of
readdir, I can see two things:
1) When beginning a search in a directory, _tfindfirst gets used,
which returns ENOENT as error if no matches are found:
https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/findfirst-functions?view=vs-2017
So from this point of view your patch is right: you make readdir()
return errno=0 which matches the normal *nix behaviors. And MinGW
does not do that.
2) On follow-up lookups, MinGW code uses _tfindnext, and actually
*enforces* errno=0 when seeing ERROR_NO_MORE_FILES. So from this
point of view the code's comment in HEAD is incorrect as of today.
The current implementation exists since 399a36a so perhaps MinGW was
not like that when dirent.c has been added in src/port/, but that's
not true today. So let's fix the comment at the same time.

Attached is an updated patch with my suggestions. Does it look fine
to you?

Yes, certainly.

Also, I think that we should credit Yuri Kurenkov for the discovery of
the issue, with yourself, Konstantin, as the author of the patch.
Are there other people involved which should be credited? Like
Grigory?

Yes, Yuri Kurenkov and Grigory Smalking did a lot in investigation of
this problem.
(the irony is that the problem detected by Yuri was caused by another
bug in pg_probackup, but we thought

1 2 >

1 - 100 of 156 matches

Mail list logo