Fork
On 2020/02/19 21:24, torikoshia wrote:
> Hi,
>
> When I ran "installcheck-bigm" tests on PostgreSQL which block size was configured to 16KB, the test failed.
> The difference is as follows:
>
> $ cat regression.diffs
> diff -U3 /home/tori/ludia_funcs/expected/pg_bigm.out /home/tori/ludia_funcs/results/pg_bigm.out
> --- /home/tori/ludia_funcs/expected/pg_bigm.out 2020-02-07 10:54:51.638653784 +0900
> +++ /home/tori/ludia_funcs/results/pg_bigm.out 2020-02-07 16:34:31.995833532 +0900
> @@ -1074,8 +1074,6 @@
> LOG: pgs2norm(): complete (set result cache): 東京都
> LOG: pgs2malloc(): buflen 2048, needed 46, maxlen 4194304
> LOG: pgs2norm(): complete (set result cache): 東京都山田太郎
> -LOG: pgs2malloc(): buflen 2048, needed 22, maxlen 4194304
> -LOG: pgs2norm(): complete (set result cache): 東京都
> LOG: pgs2malloc(): buflen 2048, needed 48, maxlen 4194304
> LOG: pgs2norm(): complete (set result cache): 東京都 山田太郎
> LOG: pgs2malloc(): buflen 2048, needed 48, maxlen 4194304
> @@ -1090,6 +1088,8 @@
> LOG: pgs2norm(): complete (set result cache): 東京都 山田太郎
> LOG: pgs2malloc(): buflen 2048, needed 34, maxlen 4194304
> LOG: pgs2norm(): complete (set result cache): 東京と京都
> +LOG: pgs2malloc(): buflen 2048, needed 22, maxlen 4194304
> +LOG: pgs2norm(): complete (set result cache): 東京都
>
> It failed at this query:
>
> ---sql/pg_bigm.sql
> 387 -- Test whether recheck is skipped expectedly when keyword length is 1 or 2
> 388 SET ludia_funcs.enable_debug TO on;
> 389 SELECT col1 FROM text_tbl
> 390 WHERE pgs2norm(col1) LIKE likequery(pgs2norm('東京都')) ORDER BY col1;
>
>
> It seems that the order of rechecking data has changed after block size was configured to 16KB.
> As the comment says, the purpose of this test is to ensure 'whether recheck is skipped expectedly'.
> If so, the order of rechecking is not important and we want to know just the recheck was done or not.
>
> Fujii-san said the current debug option was too detailed for this test and advised me to add 'terse' option for just logging the function had called.
>
> I agree with this idea and attached a patch.
> Could you review it when it's convenient
> for you?
Thanks for the patch! I'm happy to review that.
Here are some review comments.
-SET ludia_funcs.enable_debug TO on;
+SET ludia_funcs.enable_debug TO terse;
Isn't it better to add comment explaining why this option needs
to be set to terse in the regression test?
+#ifdef PGS2_DEBUG
+/* GUC variables for pgs2_enable_debug */
+static int pgs2_enable_debug = PGS2_DEBUG_OFF;
+#endif
This variable declaration is enclosed with #ifdef TEXTPORTER
and #endif, but it should not. Otherwise, when ludia_funcs is
built without TEXTPORTER flag, the compilation error happens.
-static bool pgs2_enable_debug = false;
+typedef enum pgs2_enable_debug_type
+{
+ PGS2_DEBUG_OFF,
+ PGS2_DEBUG_TERSE,
+ PGS2_DEBUG_ON
Since we use "pgs2_enable_debug" as a prefix of object name,
isn't it better to use, for example, PGS2_ENABLE_DEBUG_OFF instead
of PGS2_DEBUG_OFF, for the consistency?
elog(LOG, "pgs2norm(): complete (%s result cache): %s",
(norm_cache == NULL) ? "unset" : "set", tmp);
pfree(tmp);
}
+ else if (pgs2_enable_debug == PGS2_DEBUG_TERSE)
+ elog(LOG, "pgs2norm(): completed");
Could you tell me why you used "completed" instead of "complete" when
"terse" is specified? IMO it's better to use the same word in both
"on" and "terse" cases.
+typedef enum pgs2_enable_debug_type
+{
+ PGS2_DEBUG_OFF,
+ PGS2_DEBUG_TERSE,
+ PGS2_DEBUG_ON
+} pgs2_enable_debug_type;
For those who read the source of ludia_funcs for the first time,
it's not easy to get to know what each debug type emans. What about
adding the description of each debug type as the comment,
so that they can easily understand each debug type?
Regards,
--
Fujii Masao
NTT DATA CORPORATION
Advanced Platform Technology Group
Research and Development Headquarters